Why one should not hide a structure implementation that way?

Question

I've seen some C/C++ code using a trick to hide structure implementation using an opaque (shadow) structure of the same size:

In private.h, the exact implementation of the structure is declared:

typedef struct private_struct
{
    private_foo_t f1;
    private_bar_t b[2];
    private_baz_t *bz;
    int val;
} private_t;

#define PRIVATE_SIZE (sizeof(private_t))

In public.h, the public structure is declared to hold an opaque array of bytes:

#include "private.h"

typedef struct public_struct
{
    char opaque[PRIVATE_SIZE];
} public_t;

public_t and private_t share the same size.

Users can allocate themself storage for private implementation using the public structure:

#include <public.h>

int main(void)
{
    public_t pub;

    return public_api(&pub);
}

Implementation can access the hidden implementation:

#include "private.h"

int public_api(public_t *pub)
{
    private_t *priv = (private_t *) pub;

    return priv->val;
}

This seems a pretty neat trick to allow users to allocate storage for variables (eg. declare static variables).

I'm porting proprietary source code using this trick on various embedded system, but I'm not feeling confident in the way structure pub_t is declared.

What can be wrong with this trick ?

@doron there's an alignment issue, see the answer : http://stackoverflow.com/questions/17619015/why-you-should-not-hide-structure-implementation-that-way/17619016 — Yann Droneaud, Jul 12 '13 at 16:06
so this is a quiz type question not something that you need a genuine answer to. — doron, Jul 12 '13 at 16:09
@doron I've read carefully http://blog.stackoverflow.com/2011/07/its-ok-to-ask-and-answer-your-own-questions/ before posting my own answer to the question — Yann Droneaud, Jul 12 '13 at 16:10
@ydroneaud: Your title is a question, the body of the question is not. Even if you know the answer, it is best to phrase the question as a question. For instance, saying "But it shouldn't be done that way." is an odd thing to say. — Guvante, Jul 12 '13 at 16:11
@Guvante thanks for the comment, I've removed the offending line and a something more like a question. — Yann Droneaud, Jul 12 '13 at 16:13
First I though it was like "Attempt #3" of http://www.gotw.ca/gotw/028.htm but actually this "trick" seems just stupid to me: this code (besides using reserved keyword for struct names) does not hide anything since `public.h` _directly includes_ `private.h`... — gx_, Jul 12 '13 at 16:13
@gx_ I'm not the original author of this. But you don't need to include directly the other header, if you can generate an header with the literal size of the structure hidden structure. It's quite complex kind of preprocessing. — Yann Droneaud, Jul 12 '13 at 16:15
This really needs two questions, one for C and one for C++. The answers are *very* different. — Ben Voigt, Jul 12 '13 at 16:56

Yann Droneaud · Answer 1 · 2013-07-12T16:50:54.273

Beware of Alignment !

public_t native alignment is 1 since char are aligned to 1 byte. private_t alignment is set to the highest alignment requirement of its members, which is certainly not 1. It's probably aligned on the size of a pointer (void *), but there's a double inside a substructure which might require an alignment on 8 bytes. You may seen various kind of alignment depending on the ABI.

Let's try a example program, compiled and tested on i386/i686 with gcc (code source follow):

     kind         name       address   size   alignment   required

     type |      foo_t |         N/A |   48 |       N/A |        4 
     type |     priv_t |         N/A |   56 |       N/A |        4 
     type |      pub_t |         N/A |   56 |       N/A |        1 

   object |       u8_0 |  0xfff72caf |    1 |         1 |        1
   object |       u8_1 |  0xfff72cae |    1 |         2 |        1
   object |       u8_2 |  0xfff72cad |    1 |         1 |        1
   object |       pub0 |  0xfff72c75 |   56 |         1 |        1
   object |       u8_3 |  0xfff72c74 |    1 |         4 |        1
   object |       pub1 |  0xfff72c3c |   56 |         4 |        1
   object |       u8_4 |  0xfff72c3b |    1 |         1 |        1
   object |      priv0 |  0xfff72c00 |   56 |      1024 |        4
   object |       u8_5 |  0xfff72bff |    1 |         1 |        1
   object |      priv1 |  0xfff72bc4 |   56 |         4 |        4
   object |       u8_6 |  0xfff72bc3 |    1 |         1 |        1

  pointer |       pubp |  0xfff72c75 |   56 |         1 |        1
  pointer |      privp |  0xfff72c75 |   56 |         1 |        4  **UNALIGNED**
   object | privp->val |  0xfff72c75 |    4 |         1 |        4  **UNALIGNED**
   object | privp->ptr |  0xfff72c79 |    4 |         1 |        4  **UNALIGNED**
   object |   privp->f |  0xfff72c7d |   48 |         1 |        4  **UNALIGNED**

Source code of the test:

#include <stdalign.h>
#ifdef __cplusplus
/* you will need to pass -std=gnu++11 to g++ */
#include <cstdint>
#endif
#include <stdint.h>
#include <stdio.h>
#include <inttypes.h>

#ifdef __cplusplus
#define alignof __alignof__
#endif

#define PRINTHEADER() printheader()
#define PRINTSPACE() printspace()
#define PRINTALIGN(obj) printobjalign("object", #obj, &obj, sizeof(obj), alignof(obj))
#define PRINTALIGNP(ptr) printobjalign("pointer", #ptr, ptr, sizeof(*ptr), alignof(*ptr))
#define PRINTALIGNT(type) printtypealign(#type, sizeof(type), alignof(type))

static void
printheader(void)
{
    printf(" %8s   %10s   %18s   %4s   %9s   %8s\n", "kind", "name", "address", "size", "alignment", "required");
}

static void
printspace(void)
{
    printf(" %8s   %10s   %18s   %4s   %9s   %8s\n", "", "", "", "", "", "");
}

static void
printtypealign(const char *name, size_t szof, size_t alof)
{
    printf(" %8s | %10s | %18s | %4zu | %9s | %8zu \n", "type", name, "N/A", szof, "N/A", alof);
}

static void
printobjalign(const char *tag, const char *name, const void * ptr, size_t szof, size_t alof)
{
    const uintptr_t uintptr = (uintptr_t)ptr;
    uintptr_t mask = 1;
    size_t align = 0;

    /* get current alignment of the pointer */
    while(mask != UINTPTR_MAX) {

        if ((uintptr & mask) != 0) {
            align = (mask + 1) / 2;
            break;
        }

        mask <<= 1;
        mask |= 1;
    }

    printf(" %8s | %10s | %18p | %4zu | %9zu | %8zu%s\n",
           tag, name, ptr, szof, align, alof, (align < alof) ? "  **UNALIGNED**" : "");
}

/* a foo struct with various fields */
typedef struct foo
{
    uint8_t f8_0;
    uint16_t f16;
    uint8_t f8_1;
    uint32_t f32;
    uint8_t f8_2;
    uint64_t f64;
    uint8_t f8_3;
    double d;
    uint8_t f8_4;
    void *p;
    uint8_t f8_5;
} foo_t;

/* the implementation struct */
typedef struct priv
{
    uint32_t val;
    void *ptr;
    struct foo f;
} priv_t;

/* the opaque struct */
typedef struct pub
{
    uint8_t padding[sizeof(priv_t)];
} pub_t;

static int
test(pub_t *pubp)
{
    priv_t *privp = (priv_t *)pubp;

    PRINTALIGNP(pubp);
    PRINTALIGNP(privp);
    PRINTALIGN(privp->val);
    PRINTALIGN(privp->ptr);
    PRINTALIGN(privp->f);
    PRINTSPACE();

    return privp->val;
}

int
main(void)
{
    uint8_t u8_0;
    uint8_t u8_1;
    uint8_t u8_2;
    pub_t pub0;
    uint8_t u8_3;
    pub_t pub1;
    uint8_t u8_4;
    priv_t priv0;
    uint8_t u8_5;
    priv_t priv1;
    uint8_t u8_6;

    PRINTHEADER();
    PRINTSPACE();

    PRINTALIGNT(foo_t);
    PRINTALIGNT(priv_t);
    PRINTALIGNT(pub_t);
    PRINTSPACE();

    PRINTALIGN(u8_0);
    PRINTALIGN(u8_1);
    PRINTALIGN(u8_2);
    PRINTALIGN(pub0);
    PRINTALIGN(u8_3);
    PRINTALIGN(pub1);
    PRINTALIGN(u8_4);
    PRINTALIGN(priv0);
    PRINTALIGN(u8_5);
    PRINTALIGN(priv1);
    PRINTALIGN(u8_6);
    PRINTSPACE();

    return test(&pub0);
}

Analysis:

pub0 is allocated on the stack and is passed as argument to function test. It is aligned on 1 byte, so that, when cast'ed as a priv_t pointer, priv_t structure members are not aligned.

And that can be bad:

bad for correctness: some architectures/CPUs will silently corrupt read/write operations to unaligned memory address, some others will generate a fault. The latter is better.
bad for performance: if supported, unaligned access/load/store are still known to be poorly handled: you're likely asking the CPUs to read/write twice the memory size of the object ... you might hit the cache badly this way.

So, if you really want to hide structure content, you should take care of the alignment of the underlying structure: don't use char.

By default, use void *, or if there can be double in any members of the structure, use double. This will works until someone use a #prama or __attribute__(()) to choose an higher alignment for (a member of) the hidden structure.

Let's define correctly pub_t:

typedef struct pub
{
    double opaque[(sizeof(priv_t) + (sizeof(double) - 1)) / sizeof(double)];
} pub_t;

It might sound complex, and it is ! This way the pub_t structure will have correct alignment and be at least as big as underlying priv_t.

If priv_t was packed (with #pragma or __attribute__(())), using sizeof(priv_t)/sizeof(double), pub_t could be smaller than priv_t ... which will be even worst than the problem we were trying to solve initially. But if the structure was packed, who care of the alignment.

malloc()

If pub_t structure was allocated by malloc() instead of being allocated on stack, the alignment would not be a problem since malloc() is defined to return a memory block aligned to the greatest memory alignments of the C native types, eg. double. In modern malloc() implementations alignment could be up to 32 bytes.

This covers one of the possible errors you'll see with this approach. I'd have thought that maintainability was a bigger problem, with people adding useful new fields to the private struct and forgetting to add them to the public version. — simonc, Jul 12 '13 at 15:58
I thought that alignment problems like this only come about when you wrap the struct in a struct, or have a non-4 sized struct. — Guvante, Jul 12 '13 at 16:13
Doesn't `sizeof` automatically include the padding? See for example http://ideone.com/fZ7Cuj which appears to be padded to a 4-byte boundary. — Mark Ransom, Jul 12 '13 at 16:15
@MarkRansom you're quite correct, the compiler add padding at the end of the struct so that an array of the structure got each item aligned. But in the case of the opaque structure, this doesn't help to have correct alignment. — Yann Droneaud, Jul 12 '13 at 16:17
@ydroneaud: That padding would be returned by `sizeof`. You would have to wrap this in another struct to get bad behavior. Wrapping an opaque struct in a struct is probably bad practice in either case. — Guvante, Jul 12 '13 at 16:19
@Guvante @MarkRansom `sizeof` is not adding padding. The padding is added by the compiler at the end of the structure only if such padding is needed to that `&structure[1]` is aligned regarding structure alignment requirements. — Yann Droneaud, Jul 12 '13 at 16:22
@Guvante allocating the opaque structure on the stack is enough to have invalid alignment. — Yann Droneaud, Jul 12 '13 at 16:23
@Guvante I've not checked, but declaring a `char` global variable followed by a `pub_t` global variable might exhibit the same behavor: `pub_t` get alignmed on `char` boundary. — Yann Droneaud, Jul 12 '13 at 16:25
@ydroneaud: But `sizeof` includes any padding, including at the end. Been too long since I have played with C, I would have assumed that both those examples would be word aligned. Maybe it would be better to compact your example down a bit and show an example of a misaligned variable, rather than a bad alignment flag (which is more abstract). — Guvante, Jul 12 '13 at 16:26
@Guvante the exact wrong behavor that could arise from using badly aligned structure may vary from architecture/CPUs, so it's gonna be complex to show in less: but I will rework a bit the answer. Thanks for the comment. — Yann Droneaud, Jul 12 '13 at 16:29
There's a comment from @R that seems to explain the same as this answer: http://stackoverflow.com/questions/4440476/static-allocation-of-opaque-data-types#comment4852259_4442737 — Yann Droneaud, Jul 12 '13 at 16:57
Taking care of alignment is the most important issue, and your answer correctly answer that point. Now, unfortunately, there is still a potential issue remaining, depending on compilers and compilation settings : strict aliasing. — Cyan, Jun 26 '15 at 20:14

Ingo Leonhardt · Answer 2 · 2020-04-02T08:23:59.627

3

In most cases the nature of a internal structure is hidden from public because you want to be free to change it without having to recompile all the code that is using it. And exactly that is what you loose if you use the trick vou have mentioned and the size of private_t changes. So to be free, it's better to provide either a function like alloc_struct() that allocates a structure and returns a void * or a function that returns sizeof(private_t) so that can be used for allocating…

edited Apr 02 '20 at 08:23

answered Jul 12 '13 at 16:06

Ingo Leonhardt

9,435
2
24
33

1

Do you mean "free to change *it*"? – chux - Reinstate Monica Mar 26 '20 at 17:03

score 3 · Answer 3 · answered Jul 12 '13 at 17:02

Here's what is wrong with it in C++. From 3.8 [basic.life]:

The lifetime of an object of type T begins when:

storage with the proper alignment and size for type T is obtained, and

if the object has non-trivial initialization, its initialization is complete.

and later

For an object of a class type with a non-trivial destructor, the program is not required to call the destructor explicitly before the storage which the object occupies is reused or released; however, if there is no explicit call to the destructor or if a delete-expression (5.3.5) is not used to release the storage, the destructor shall not be implicitly called and any program that depends on the side effects produced by the destructor has undeﬁned behavior.

Others have already pointed out the potential alignment issues, which also exist in C. But in C++ initialization is a special problem. The public user isn't performing any, so you can only cast the pointer to the private type and use it if the private type has no initialization. There's a parallel problem with destruction -- you force the private object to have trivial destruction.

Which clearly is why you've written private_baz_t *bz; when you should be using a smart pointer.

The only "benefits" this trick buys you are memory leaks and lack of exception safety -- all the things RAII is designed to protect against. Use the p/impl pattern instead, which actually provides a compilation firewall and improves build times.

Why one should not hide a structure implementation that way?

3 Answers3

Linked