[CDRIVER-2880] Struct alignment is less then the default packing size on Windows x86 builds Created: 08/Nov/18  Updated: 22/Mar/19  Resolved: 19/Feb/19

Status: Closed
Project: C Driver
Component/s: libbson
Affects Version/s: 1.13.0
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Sascha Zelzer Assignee: Kevin Albertson
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Visual Studio 2017 15.8.8
x86 (32-bit) target architecture


Issue Links:
Related
related to CDRIVER-2957 Consider adding an option to disable ... Closed

 Description   

On Windows (with ___MSC_VER defined), the bson-macros.h header defines BSON_ALIGN_OF_PTR to 4 for 32-bit target architectures.

With BSON_EXTRA_ALIGN not defined, this results in structures being declared with an alignment of 4 which is less then the default packing size (which is 8, unless overridden using /Zp which is not recommended in practice) and will be ignored.

When using libbson in a C++ project and hence compiling libbson headers in C++ mode, this prints the following compiler warning:

\include\libbson-1.0\bson\bson-types.h(296,0): Warning C4359: '_bson_value_t': Alignment specifier is less than actual alignment (8), and will be ignored.

 



 Comments   
Comment by Sascha Zelzer [ 22/Mar/19 ]

Hi Kevin,

now I have to apologize for my late response.

Thanks a lot for your detailed answer and investigation. It helped me understand the problem and potential work-arounds better.

Best,

Sascha

Comment by Kevin Albertson [ 11/Feb/19 ]

Hi Sascha,

Apologies for the delayed response.

It's odd that the warning only occurred when compiling C++. I've determined with tests that the behavior is the same for C, but reason MSVC just does not produce the warning for C.

I am explicitly setting ENABLE_EXTRA_ALIGNMENT to OFF in my CMake configuration, which - if I understood the past discussions correctly - already breaks the old ABI

Right, changing the alignment under ENABLE_EXTRA_ALIGNMENT=OFF was necessary because it's an ABI breaking change. But we can't break ABI again under the same configuration. If someone configures the C driver with the same options and builds libbson/libmongoc, they should be able to drop them in without recompiling their application.

But actually, according to the warning and MSDN documentation, this is going to be ignored and a value of 8 is used (just like for the Win64 platform).

bson_value_t has a int64_t member. The rules of structure alignment for MSVC say:

Unless overridden with __declspec(align(#)), the alignment of a structure is the maximum of the individual alignments of its member(s).
[...]
__declspec(align(#)) can only increase alignment restrictions.

Since int64_t has 8 byte alignment, bson_value_t gets 8 byte alignment (regardless of 64 or 32 bit). The alignment specifier of 4 is ignored, triggering the warning.

But if we change BSON_ALIGN_OF_PTR to 8 for 32 bit Windows, other structs would change alignment (regardless of /Zp). bson_error_t is one of them:

BSON_ALIGNED_BEGIN (8)
typedef struct _bson_error_t {
   uint32_t domain;
   uint32_t code;
   char message[BSON_ERROR_BUFFER_SIZE];
} bson_error_t BSON_ALIGNED_END (8);

If compiling on 32 bit Windows, bson_error_t is 4 byte aligned. Changing BSON_ALIGN_OF_PTR to 8 would change it to 8. Here's a small repro of that behavior.

In conclusion, we can't change the value of BSON_ALIGN_OF_PTR under ENABLE_EXTRA_ALIGNMENT=OFF. But, we are considering a more complete solution. CDRIVER-2957 proposes adding a separate configuration option to disable alignment specifiers entirely.

In the meantime, I know it's not ideal but one option is to silence the warning for the bson headers, something like:

#pragma warning( push )
#pragma warning( disable : 4359)
#include <bson/bson.h>
#pragma warning( pop )

Best,
Kevin

Comment by Sascha Zelzer [ 11/Dec/18 ]

Hi, thanks for taking time to look at this.

I am confused now. I am explicitly setting ENABLE_EXTRA_ALIGNMENT to OFF in my CMake configuration, which - if I understood the past discussions correctly - already breaks the old ABI. However, with BSON_EXTRA_ALIGN now not defined (being ABI incompatible), the default BSON_ALIGN_OF_PTR is set to 4 for Win32 platforms using MSVC. But actually, according to the warning and MSDN documentation, this is going to be ignored and a value of 8 is used (just like for the Win64 platform). So changing the value from 4 to 8 is not going to change anything, except silencing the compiler warning about this. Did you misunderstand this totally wrong.

I can patch this myself of course, which would be my work-around, but it would nice to actually understand why it is like this upstream and eventually have it fixed.

Thanks!

Comment by Kevin Albertson [ 06/Dec/18 ]

Hi saszel, thank you for the report. Unfortunately we can't increase the alignment of bson_t without making an ABI break. That's the same reason the BSON_EXTRA_ALIGNMENT was created as an opt-in option (see the discussion in CDRIVER-596). Alas, as you've discovered, the libbson specifies aligning at 4 byte boundaries on Windows 32 bit.

I want to check whether or not this is a real problem or whether this warning is harmless. If your C++ project compiles with 8-byte alignment for bson_t, but you're linking against libbson that's been compiled with 4-byte alignment, that's likely a problem.

In the meantime, is your workaround sufficient?

Comment by Sascha Zelzer [ 08/Nov/18 ]

Oh well, I was struggling with the formatting when creating this bug and tried Ctrl-Enter, which created the bug although I was not finished writing the description. And it seems I cannot edit the description afterwards

So in addition, curiously, the warning does not appear when compiling C code. Unless I overlooked sometlhing, I think just using

 
#define BSON_ALIGN_OF_PTR 8

independent of the target architecture would just do the trick.

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