[CDRIVER-1028] bson-types.h / bson-macros.h set the aligned attribute for GCC incorrectly Created: 03/Dec/15  Updated: 28/Jun/17  Resolved: 06/Jan/17

Status: Closed
Project: C Driver
Component/s: libbson
Affects Version/s: 1.3.0-beta0
Fix Version/s: 1.6.0

Type: Bug Priority: Minor - P4
Reporter: Theodore B Assignee: A. Jesse Jiryu Davis
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

GCC 5.2.1 / SUSE Linux



 Description   

In libbson/src/bson/bson-macros.h, it appears that for GCC, instead of:

#  define BSON_ALIGNED_BEGIN(_N)
#  define BSON_ALIGNED_END(_N) __attribute__((aligned (_N)))

We should use:

#  define BSON_ALIGNED_BEGIN(_N) __attribute__((aligned (_N)))
#  define BSON_ALIGNED_END(_N)

I noticed this because I was unable to declare the array

bson_iter_t b[1];

with compiler error "alignment of array elements is greater than element size". With the current code we sill have:

// Checking size, expecting "bson_iter_t: 128"
printf( "\nbson_iter_t: %zu\n", sizeof(bson_iter_t) );    // Output "bson_iter_t: 80"

Example:

#include "bson.h"
 
// Example definition for bson_iter_t:
//
// BSON_ALIGNED_BEGIN (128)
// typedef struct
// {
//    const uint8_t *raw;      /* The raw buffer being iterated. */
//    uint32_t       len;      /* The length of raw. */
//    uint32_t       off;      /* The offset within the buffer. */
//    uint32_t       type;     /* The offset of the type byte. */
//    uint32_t       key;      /* The offset of the key byte. */
//    uint32_t       d1;       /* The offset of the first data byte. */
//    uint32_t       d2;       /* The offset of the second data byte. */
//    uint32_t       d3;       /* The offset of the third data byte. */
//    uint32_t       d4;       /* The offset of the fourth data byte. */
//    uint32_t       next_off; /* The offset of the next field. */
//    uint32_t       err_off;  /* The offset of the error. */
//    bson_value_t   value;    /* Internal value for various state. */
// } bson_iter_t
// BSON_ALIGNED_END (128);
 
 
// Incorrectly defined type Bar
typedef struct
{
    char c;
    int  i;
} Bar __attribute__((aligned(128)));
 
 
// Correctly defined type Foo
typedef struct __attribute__((aligned (128)))
{
    char c;
    int  i;
} Foo;
 
int main (int argc, char *argv[])
{
 
    printf( "bson_t: %zu\n",sizeof(bson_t    ) );   // Output:  "bson_t: 128"
//     printf( "bson_t[1]: %zu\n",sizeof(bson_t[1] ) ); // GCC 5.2.1 compiler error: alignment of array elements is greater than element size
    
    printf( "\nbson_iter_t: %zu\n",sizeof(bson_iter_t   ) );    // Output "bson_iter_t: 80"
//     printf( "bson_iter_t[1]: %zu\n",sizeof(bson_iter_t[1]) ); // GCC 5.2.1 compiler error: alignment of array elements is greater than element size
 
    printf( "\nBar: %zu\n",sizeof(Bar)    );   // Output:  "Bar: 8"
//     printf( "Bar[1]: %zu\n",sizeof(Bar[1]) ); // GCC 5.2.1 compiler error: alignment of array elements is greater than element size
    
    printf( "\nFoo: %zu\n",sizeof(Foo)    );   // Output:  "Foo: 128"
    printf( "Foo[1]: %zu\n",sizeof(Foo[1]) );   // Output:  "Foo[1]: 128"
 
    return 0;
}

*****

I would appreciate it if you could still include me in the bug hunt for 3.2 even though I'm technically 1 day past the deadline ... I tried to write it up nice to win you guys over!! Happy Hunting!!



 Comments   
Comment by A. Jesse Jiryu Davis [ 28/Jun/17 ]

Yes, unfortunately we can't fix this without an ABI break, which didn't seem worth the trouble. Breaking ABI would require a variety of chores, like maintaining Debian and Fedora packages that adhere to both the old and new ABI for the foreseeable future. Not an ideal situation, but it's better to continue with what we have.

Comment by Theodore B [ 27/Jun/17 ]

I have not worked with these drivers in a while, so I'm not sure what the latest status is, but as far as I recall the part that did not work was declaring an array of bson_t, bson_iter_t, etc., resulting in GCC complaining that "alignment of array elements is greater than element size". The results were initial confusion, time wasted investigating, and ultimately writing code that was less concise than it needed to be when all I wanted was a simple array.

Not sure why you wouldn't want to just fix it if it has not not been fixed. It didn't seem like a major rewrite from my perspective. Am I missing something?

To be specific, none of the following would work:

bson_t blurb[3];
bson_iter_t blarb[3];
 
printf( "bson_t[3]: %zu\n",sizeof(bson_t[3] ) );
printf( "bson_iter_t[3]: %zu\n",sizeof(bson_iter_t[3]) );

Comment by A. Jesse Jiryu Davis [ 06/Jan/17 ]

The alignment specifiers do in fact work as designed: they control struct alignment on all platforms.

If we began again, we'd probably choose not to specify alignment for any libbson structs, but the code we have works and we must preserve it for ABI.

Comment by A. Jesse Jiryu Davis [ 06/Jan/17 ]

Let's do an experiment, I'll try putting the alignment specifier at every possible position in the typedef.

#ifdef _MSC_VER
#define ALIGN(N) __declspec(align (N))
#define ALIGNOF(T) __alignof(T)
#else
#define ALIGN(N) __attribute__((aligned (N)))
#define ALIGNOF(T) _Alignof(T)
#endif
 
   ALIGN(128) typedef struct { int i; } a;
   typedef ALIGN(128) struct { int i; } b;
   typedef struct ALIGN(128) { int i; } c;
#ifndef _MSC_VER
   typedef struct { int i; } d ALIGN(128) ;
#endif
 
   printf ("a sizeof: %3d, alignof: %3d\n", (int) sizeof (a), (int) ALIGNOF (a));
   printf ("b sizeof: %3d, alignof: %3d\n", (int) sizeof (b), (int) ALIGNOF (b));
   printf ("c sizeof: %3d, alignof: %3d\n", (int) sizeof (c), (int) ALIGNOF (c));
#ifndef _MSC_VER
   printf ("d sizeof: %3d, alignof: %3d\n", (int) sizeof (d), (int) ALIGNOF (d));
#endif

The typedef for struct "d" doesn't even compile for MSVC, so it's commented-out.

GCC 4.6 won't compile this because _Alignof isn't implemented there.

Clang and GCC 4.8 print:

a sizeof:   4, alignof: 128
b sizeof:   4, alignof: 128
c sizeof: 128, alignof: 128
d sizeof:   4, alignof: 128

MSVC prints:

a sizeof: 128, alignof: 128
b sizeof: 128, alignof: 128
c sizeof: 128, alignof: 128

Our current BSON_ALIGN macros put the specifier at the beginning like "a" for MSVC, and put it at the end like "d" for GCC and Clang. So right now, our MSVC code sets the size and alignment for structs, and our GCC / Clang code sets alignment only.

Microsoft says sizeof >= alignof, which matches what I'm seeing: https://msdn.microsoft.com/en-us/library/83ythb65.aspx

I can't find anything that says GCC and Clang allow sizeof < alignof, but that appears to be true.

Comment by A. Jesse Jiryu Davis [ 07/Dec/15 ]

It looks like you're right, the interaction of "typedef struct" and our alignment specifier syntax is not working as expected; we'll investigate further and decide what version we can safely fix this in without breaking the existing ABI.

Comment by A. Jesse Jiryu Davis [ 03/Dec/15 ]

Thanks for the report. I don't believe the 3.2 bughunt applies to drivers, only to the MongoDB server. In any case we'll take a look at this issue.

Generated at Wed Feb 07 21:11:20 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.