tags:

views:

264

answers:

5

I have seen a codebase recently that I fear is violating alignment constraints. I've scrubbed it to produce a minimal example, given below. Briefly, the players are:

  • Pool. This is a class which allocates memory efficiently, for some definition of 'efficient'. Pool is guaranteed to return a chunk of memory that is aligned for the requested size.

  • *Obj_list*. This class stores homogeneous collections of objects. Once the number of objects exceeds a certain threshold, it changes its internal representation from a list to a tree. The size of *Obj_list* is one pointer (8 bytes on a 64-bit platform). Its populated store will of course exceed that.

  • Aggregate. This class represents a very common object in the system. Its history goes back to the early 32-bit workstation era, and it was 'optimized' (in that same 32-bit era) to use as little space as possible as a result. Aggregates can be empty, or manage an arbitrary number of objects.

In this example, Aggregate items are always allocated from Pools, so they are always aligned. The only occurrences of *Obj_list* in this example are the 'hidden' members in Aggregate objects, and therefore they are always allocated using placement new. Here are the support classes:

class Pool
{
public:
   Pool();
   virtual ~Pool();
   void *allocate(size_t size);
   static Pool *default_pool();   // returns a global pool
};

class Obj_list
{
public:
   inline void *operator new(size_t s, void * p) { return p; }

   Obj_list(const Args *args);
   // when constructed, Obj_list will allocate representation_p, which
   // can take up much more space.

   ~Obj_list();

private:
   Obj_list_store *representation_p;
};

And here is Aggregate. Note that member declaration *member_list_store_d*:

// Aggregate is derived from Lesser, which is twelve bytes in size
class Aggregate : public Lesser
{
public:
   inline void *operator new(size_t s) {
      return Pool::default_pool->allocate(s);
   }

   inline void *operator new(size_t s, Pool *h) {
      return h->allocate(s);
   }

public:

   Aggregate(const Args *args = NULL);
   virtual ~Aggregate() {};

   inline const Obj_list *member_list_store_p() const;

protected:
   char member_list_store_d[sizeof(Obj_list)];
};

It is that data member that I'm most concerned about. Here is the pseudocode for initialization and access:

Aggregate::Aggregate(const Args *args)
{
   if (args) {
      new (static_cast<void *>(member_list_store_d)) Obj_list(args);
   }
   else {
      zero_out(member_list_store_d);
   }
}

inline const Obj_list *Aggregate::member_list_store_p() const
{
   return initialized(member_list_store_d) ? (Obj_list *) &member_list_store_d : 0;
}

You may be tempted to suggest that we replace the char array with a pointer to the *Obj_list* type, initialized to NULL or an instance of the class. This gives the proper semantics, but just shifts the memory cost around. If memory were still at a premium (and it might be, this is an EDA database representation), replacing the char array with a pointer to an *Obj_list* would cost one more pointer in the case when Aggregate objects do have members.

Besides that, I don't really want to get distracted from the main question here, which is alignment. I think the above construct is problematic, but can't really find more in the standard than some vague discussion of the alignment behavior of the 'system/library' new.

So, does the above construct do anything more than cause an occasional pipe stall?

Edit: I realize that there are ways to replace the approach using the embedded char array. So did the original architects. They discarded them because memory was at a premium. Now, if I have a reason to touch that code, I'll probably change it.

However, my question, about the alignment issues inherent in this approach, is what I hope people will address. Thanks!

+1  A: 

The alignment will be picked by the compiler according to its defaults, this will probably end up as four-bytes under GCC / MSVC.

This should only be a problem if there is code (SIMD/DMA) that requires a specific alignment. In this case you should be able to use compiler directives to ensure that member_list_store_d is aligned, or increase the size by (alignment-1) and use an appropriate offset.

Andrew Grant
- Aggregate is aligned by Pool.- Aggregate is derived from Lesser, which is twelve bytes.- member_list_store_d is a char array in Aggregate.Are you saying that the compiler will four-byte align that char array member? If so, presumably it would eight-byte align on a 64-bit platform?
Don Wakefield
A: 

Allocate the char array member_list_store_d with malloc or global operator new[], either of which will give storage aligned for any type.

Edit: Just read the OP again - you don't want to pay for another pointer. Will read again in the morning.

fizzer
I guess I don't understand. The char array *member_list_store_d* is a member var of *Aggregate*. We use placement new *into* this char array to construct the *Obj_list* object. What are you suggesting I do with malloc or global operator new[] to this existing array data member?
Don Wakefield
As a data member, the char array is not guaranteed to have any particular alignment. You could instead make member_list_store_d a char*, and allocate it with malloc(sizeof(Obj_list)) or new char[sizeof(Obj_list)] in Aggregate's ctor. That gets you your alignment guarantee with minimal disruption.
fizzer
Note that strictly, global operator new[] for char* guarantees alignment only for types which are small enough to fit into the array you allocated. That's enough for all practical purposes (which is why it's all that's guaranteed). But it does mean that "new char[2]" isn't necessarily 4-aligned.
Steve Jessop
... unlike malloc(2). Assuming a platform where there exists a type requiring 4-alignment.
Steve Jessop
Do you have a cite? I'm looking at 3.7.3.1 'The pointer returned shall be suitably aligned so that it can be converted to apointer of any complete object type and then used to access the object or array in the storage allocated'. But it's a big document - I could be missing something.
fizzer
Could be the 'as-if' rule - a conforming program can't tell that it's not aligned?
fizzer
+1  A: 

If you want to ensure alignment of your structures, just do a

// MSVC
#pragma pack(push,1)

// structure definitions

#pragma pack(pop)

// *nix
struct YourStruct
{
    ....
} __attribute__((packed));

To ensure 1 byte alignment of your char array in Aggregate

PiNoYBoY82
This software runs primarily on Unix/Linux. Is that pragma available on compilers that target *nix?
Don Wakefield
Changed my posting for *nix
PiNoYBoY82
Thanks, @PiNoYBoY82, I'll read up on that.
Don Wakefield
+1  A: 

Can you simply have an instance of Obj_list inside Aggregate? IOW, something along the lines of

class Aggregate : public Lesser { ... protected: Obj_list list; };

I must be missing something, but I can't figure why this is bad.

As to your question - it's perfectly compiler-dependent. Most compilers, though, will align every member at word boundary by default, even if the member's type does not need to be aligned that way for correct access.

Arkadiy
As @fizzer.myopen.com posted elsewhere, this approach was apparently discarded by the original architects because it would have cost an extra pointer when the *Obj_list* object was constructed.
Don Wakefield
How so? The question said "replacing the char array with a pointer to an *Obj_list* would cost one more pointer in the case when Aggregate objects do have members". I am talking about _instance_ of Obj_list, not pointer.
Arkadiy
Sorry, I misunderstood. So an Obj_list (as implemented) will construct a collection object in secondary store. The array is used to avoid constructing it if no objects are supplied. Again, I find this solution reasonable, but the question is about alignment of this construct, not alternatives.
Don Wakefield
Oh, my answer to alignment issue is quite obvious, I think. Just don't depend on it. And I am trying to show how to get rid of the dependency. A possible constructor for Obj_list would set the pimpl to NULL and await the first insertion before doing anything else.
Arkadiy
@Arkadiy, thanks for the input. I'd already thought of replacements, including the one you just mentioned. This topic is just to discuss the implications of the described 'idiom'.
Don Wakefield
+2  A: 

Ok - had a chance to read it properly. You have an alignment problem, and invoke undefined behaviour when you access the char array as an Obj_list. Most likely your platform will do one of three things: let you get away with it, let you get away with it at a runtime penalty or occasionally crash with a bus error.

Your portable options to fix this are:

  • allocate the storage with malloc or a global allocation function, but you think this is too expensive.
  • as Arkadiy says, make your buffer an Obj_list member:

    Obj_list list;
    

but you now don't want to pay the cost of construction. You could mitigate this by providing an inline do-nothing constructor to be used only to create this instance - as posted the default constructor would do. If you follow this route, strongly consider invoking the dtor

list.~Obj_list();

before doing a placement new into this storage.

Otherwise, I think you are left with non portable options: either rely on your platform's tolerance of misaligned accesses, or else use any nonportable options your compiler gives you.

Disclaimer: It's entirely possible I'm missing a trick with unions or some such. It's an unusual problem.

fizzer
I've been bugged by it ever since I ran across it. It's been in place for nearly twenty years in *shipping* code, but I *still* feel the urge to change it for one of the options you (and I, and others) have enumerated. Any response to @Andrew's first post? He seems to imply this 'idiom' is 'safe'.
Don Wakefield
I can't read that interpretation into his post: he says that it may be a problem on some platforms. If the code has been working for 20 years without incident, your platform clearly isn't one of them. Undefined behaviour is simply undefined. The program isn'*t required* to crash.
fizzer