12

In the following code I have been meticulous in the following of the standard's words (plus in the light of the wording of P0137) on object lifetimes.

Note that all memory allocation is through suitably-aligned storage of type unsigned char, as per P0137.

Note also that Foo is a POD, with a trivial constructor.

Questions:

A. If I have misunderstood the standard, and there is any UB here, please kindly point it out (or alternatively confirm that there is no UB)

B. Are the initialisations at A, B, C, D, E, F strictly necessary in light of the fact that the construction is trivial, and performs no actual initialisation. If so, please indicate which part of the standard contradicts or clarifies [object.lifetime] in this regard.

code:

#include <memory>

// a POD with trivial constructor
struct Foo 
{
  int x;
};

struct destroy1
{
  void operator()(Foo* p)
  {
    // RAII to guarantee correct destruction order
    auto memory = std::unique_ptr<unsigned char[]>(reinterpret_cast<unsigned char*>(p));
    p->~Foo(); // A
  }
};
std::unique_ptr<Foo, destroy1> create1()
{
  // RAII to guarantee correct exception handling
  auto p = std::make_unique<unsigned char[]>(sizeof(Foo));
  auto pCandidate = reinterpret_cast<Foo*>(p.get());
  new (pCandidate) Foo(); // B
  return std::unique_ptr<Foo, destroy1>(reinterpret_cast<Foo*>(p.release()), 
                                        destroy1());
}

struct call_free
{
  void operator()(void *p) const { std::free(p); } 
};
using malloc_ptr = std::unique_ptr<unsigned char, call_free>;

struct destroy2
{
  void operator()(Foo *pfoo) const {
    // RAII to guarantee correct destruction order
    auto memory = malloc_ptr(reinterpret_cast<unsigned char*>(pfoo));
    pfoo->~Foo(); // C
  }
};

std::unique_ptr<Foo, destroy2> create2()
{
    // RAII to guarantee correct exception handling
  auto p = malloc_ptr(reinterpret_cast<unsigned char*>(std::malloc(sizeof(Foo))));
  auto pCandidate = reinterpret_cast<Foo*>(p.get());
  new (pCandidate) Foo(); // D
  return std::unique_ptr<Foo, destroy2>(reinterpret_cast<Foo*>(p.release()), 
                                        destroy2());
}

struct nodelete {
  void operator()(Foo * p) {
    p->~Foo();  // E
  }
};

std::shared_ptr<Foo> provide()
{
  alignas(Foo) static unsigned char  storage[sizeof(Foo)];

  auto make = [] {
    auto p = reinterpret_cast<Foo*>(storage);
    new (p) Foo (); // F
    return std::shared_ptr<Foo>(p, nodelete());
  };

  static std::shared_ptr<Foo> pCandidate = make();

  return pCandidate;
}


int main()
{
  auto foo1 = create1();
  auto foo2 = create2();
  auto foo3 = provide();

  foo1->x = 1;
  foo2->x = 2;
  foo3->x = 3;
}
Morwenn
  • 21,684
  • 12
  • 93
  • 152
Richard Hodges
  • 68,278
  • 7
  • 90
  • 142
  • 1
    Calling the destructor is only required if the program depends on side-effects of the destructor (C++14 [basic.life]/4), which is not the case for `Foo`. So you can omit all the destruction code. Releasing storage ends the lifetime of any objects that are in the storage (with UB only if they have such destructors). – M.M Dec 02 '16 at 14:36
  • 3
    B, D, and F are required as per [intro.object]/1 list of things that create an object. Technically I think F should be `auto p = new(storage) Foo();` instead of the two lines you have, and similarly for B and D - you're meant to use the return value of placement new. I'm not aware of any requirement that `std::make_unique(sizeof(Foo));`'s storage be suitably aligned for `Foo`, so the B case might have an alignment problem. I don't have time right now to write a rigorous answer but will revisit after the weekend. Good post – M.M Dec 02 '16 at 14:42
  • 2
    I'd suggest rolling back your last edit as it makes it very difficult to understand what the question is, as it now starts with half-answer half-commentary. – Barry Dec 02 '16 at 20:23
  • 1
    @Barry so done. – Richard Hodges Dec 02 '16 at 20:47

3 Answers3

6

On create1

std::unique_ptr<Foo, destroy1>(reinterpret_cast<Foo*>(p.release()), destroy1());

This doesn't work, because you're using the wrong pointer.

p.release() thinks it points to an unsigned char[]. However, that's not the object you want to point to. What you want to point to is the object that lives inside this array, the Foo you've created.

So you are now subject to [basic.life]/8. The gist of that is that you can only use the previous pointer as a pointer to the new object if they are of the same type. Which they're not in your case.

Now, I could tell you to launder the pointer, but the more reasonable way to handle this is to just store the pointer returned by the placement-new call:

auto p = std::make_unique<unsigned char[]>(sizeof(Foo));
auto ret = std::unique_ptr<Foo, destroy1>(new(p.get()) Foo(), destroy1());
p.release();
return ret;

That pointer will always be correct.

Your use of of placement-new is not optional. [intro.object]/1 tells us:

An object is created by a definition (3.1), by a new-expression (5.3.4), when implicitly changing the active member of a union (9.3), or when a temporary object is created (4.4, 12.2).

When you allocate an unsigned char[], that's the object you have created in that storage. You cannot simply pretend that it is a Foo, just because Foo is an aggregate. [intro.object]/1 doesn't allow that. You must explicitly create that object via one of the mechanisms listed above. Since you can't use a definition, union member activation, or temporary objects with arbitrary memory buffers to create objects from existing storage, the only recourse you have to create objects is a new-expression.

Specifically, placement-new.

As for delete1, you do need a custom deleter, since the default deleter will call delete on the Foo pointer. Your code is as follows:

auto memory = std::unique_ptr<unsigned char[]>(reinterpret_cast<unsigned char*>(p));
p->~Foo();

unsigned char[] has some special logic to it, in terms of how it behaves when objects are allocated in their storage, thanks to [intro.object]/3-4. If the object entirely overlays the storage of the unsigned char[], then it functions as if the object were allocated within the array. That means that the unsigned char[] is still technically there; it has not destroy the byte array.

As such, you can still delete the byte array, which your code here does.

On create2

This is also wrong, due to further violations of [basic.life]/8. A fixed version would be similar to the above:

auto p = malloc_ptr(reinterpret_cast<unsigned char*>(std::malloc(sizeof(Foo))));
auto ret std::unique_ptr<Foo, destroy2>(new(p.get()) Foo(), destroy2());
p.release();
return ret;

Unlike new-expressions, malloc never creates an object via [intro.object]/1; it only acquires storage. As such, placement-new is again required.

Similarly, free just releases memory; it doesn't deal with objects. So your delete2 is essentially fine (though the use of malloc_ptr there makes it needlessly confusing).

On provide

This has the same [basic.life]/8 problems that the rest of your examples have:

alignas(Foo) static unsigned char storage[sizeof(Foo)];
static auto pCandidate = std::shared_ptr<Foo>(new(storage) Foo(), nodelete());
return pCandidate;

But other than that, it's fine (so long as you don't break it elsewhere). Why? That's complex.

[basic.start.term]/1 tells us that static objects are destroyed in the reverse order of their initialization. And [stmt.decl]/4 tells us that block-scoped static objects are initialized in the order they are encountered in a function.

Therefore, we know that pCandidate will be destroyed before storage. So long as you don't keep a copy of that shared_ptr in a static variable, or otherwise fail to destroy/reset all such shared objects before termination, you should be fine.


That all being said, using blocks of unsigned char is really pre-C++11. We have std::aligned_storage and std::aligned_union now. Use them.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • Thank you Nichol. I appreciate the comment about aligned storage. I wanted to avoid that because it was tangential to what I wanted to find out. Understood re the result of placement new. I am guessing that this is to allow the compiler to put some magic prior to the objects address should it feel the need. Of the above, the most 'safe' is using the result of malloc (because this is storage and not an object). Question B then remains - do I *really* need to call placement new - since the object is a POD, it's address *must* be the same as the buffer, *and* it does not require initialisation – Richard Hodges Dec 02 '16 at 18:39
  • 1
    @RichardHodges: "*Of the above, the most 'safe' is using the result of malloc (because this is storage and not an object).*" I fail to see why that is particularly "safe", relative to anything else. If you want to just allocate storage, you can call `operator new` directly. As for the question of placement-new, I've updated the answer to address that. – Nicol Bolas Dec 02 '16 at 19:00
  • 1
    All of the above I am asking because sometimes c++ shares struct definitions with c (e.g. mysql.h, openssl.h). c can make a struct instance available to c++ (they share exactly the same layout, by definition). but the c struct will not have had a placement new called upon it. By strict reading of the standard this implies that c++ may not treat the instance as an object while c may. This seems at odds with the wording around standard layout and PODs (at least to me). Again, I am aware of aligned_storage. That is not my concern here. – Richard Hodges Dec 02 '16 at 19:11
  • 2
    ...forgot to say, it's a great answer BTW. Very informative. Thank you. – Richard Hodges Dec 02 '16 at 19:14
  • 1
    @RichardHodges: The interoperation between C and C++ is an implementation-defined area. The interface between C and C++ functions exist (in the sense that `extern C` is a thing), but whether what C does with memory is good enough to be an object for C++ purposes is implementation-defined. – Nicol Bolas Dec 02 '16 at 19:19
  • 2
    That is a shocking revelation! It is essentially saying that calling out to any c library (other than presumably parts of the c runtime library) is implementation-defined (i.e. unreliable). This surely cannot be the intention of the standards committee? This seems to me to be too important a thing to simply dismiss it as "implementation defined". c-standards-conforming libraries either work with c++ or they do not - otherwise the entire edifice of computing is built on wishes and rainbows, is it not? – Richard Hodges Dec 02 '16 at 19:32
  • 3
    `unsigned char[]` is special. It *provides storage for* an object you placement-`new` in it ([\[intro.object\]/3](https://timsong-cpp.github.io/cppwp/intro.object#3)), and so that object is *nested within* said `unsigned char[]` ([\[intro.object\]/4](https://timsong-cpp.github.io/cppwp/intro.object#4)), which means that the placement-`new` doesn't end the lifetime of the `unsigned char[]` ([\[basic.life\]/1.4](https://timsong-cpp.github.io/cppwp/basic.life#1.4)) because its storage is being reused by an object nested within it.. – T.C. Dec 02 '16 at 19:45
  • 2
    Since there is no lifetime-ending, [basic.life]/8 is not applicable. The problem here is that the result of the `reinterpret_cast` cast doesn't point to the `Foo` object because the objects at issue are not pointer-interconvertible (see [\[expr.static.cast\]/13](https://timsong-cpp.github.io/cppwp/expr.static.cast#13)). Instead, "the pointer value is unchanged by the conversion", which means it still points to the first element of the `unsigned char[]` array, even though its type is `Foo *`. – T.C. Dec 03 '16 at 04:20
1

If you take seriously Core Issue 1776 and the never voted for idea that "that malloc alone is not sufficient to create an object", then you have to take seriously these ideas:

  • unions were not usable without UB in C++ until quite recently, as the lifetime of their members was not defined
  • string literals cannot be examined, as their lifetime is never started

and many other deeper more difficult controversies and contradictions, like what is a lvalue, an object, is lifetime a property of a preexisting object (that exists outside it's life), etc.

Yet I don't see people taking at least the two bullet points seriously. Why would the claim in the DR be taken seriously then?

curiousguy
  • 8,038
  • 2
  • 40
  • 58
0

"Core Issue 1776: Replacement of class objects containing reference members" is based on an obvious and serious interpretation error, and as such should be dismissed. The error is here:

Drafting note: this maintains the status quo that malloc alone is not sufficient to create an object

This goes against the status quo that "malloc alone" is indeed sufficient to create an object, as malloc returns suitably aligned storage, which is and has always been sufficient to create an object.

A core issue is not the standard. It is an opinion about the standard. That opinion is in error.

curiousguy
  • 8,038
  • 2
  • 40
  • 58
  • 1
    You'll be amazed to learn that the standards committee really does intend that malloc alone is not sufficient to create a POD. Writing to a structure that was created by some other language (e.g. C) is IB or UB depending on whether it's an array or not. For what it's worth, I think they are unhinged, but there you are. Apparently in c++, memory is not really memory. – Richard Hodges Jan 30 '17 at 07:32
  • @RichardHodges The C++ committee is simply ... gone. You see what I mean. – curiousguy Nov 09 '17 at 06:58
  • @RichardHodges I admit that enough committee members say that so it's the consensus, what I don't accept is that it's the intent status quo. If it had been the intent (and not a stupid mistake like the fact using string literals has UB), it would have been documented a silent change WRT identical C code; and inane code with sequences as `//* */` was listed a code possibly broken when compiling in as C++! The issue I have is that ppl don't take seriously the fact you can't refer to a base class w/ a ptr but they say you can't write C/C++ code. – curiousguy Oct 29 '19 at 19:55