[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 |
||
| Issue Links: |
|
||||||||
| 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:
|
| 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.
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.
bson_value_t has a int64_t member. The rules of structure alignment for MSVC say:
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:
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. In the meantime, I know it's not ideal but one option is to silence the warning for the bson headers, something like:
Best, | ||||||||||
| 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 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
independent of the target architecture would just do the trick. |