[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:
We should use:
I noticed this because I was unable to declare the array
Example:
***** 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:
| ||||||||||||||||||||||||||||
| 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.
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:
MSVC prints:
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. |