As pointed out in https://github.com/llvm/llvm-project/issues/61336, objects with
unnamed bitfields aren't be uniquely representable.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM
I got confused at first why this fix is done in a function named getSubobjectSizeInBits, and why do we calculate size to produce a boolean value in the end. Then I realized that this function returns number of bits of value representation, or std::nullopt if there are any padding bits, and we take advantage of the latter to implement hasUniqueObjectRepresentations. I guess I'm either missing some context or that function could be named better (not in this patch, of course).
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
2822 | I think the check for a non-zero bit-field width might not be completely correct in cases where it's used to split an allocation unit. I suggested a test case below. | |
clang/test/SemaCXX/type-traits.cpp | ||
2886–2889 | I think there's one more test to add: struct UnnamedEmptyBitfieldSplit { short named; int : 0; short also_named; }; static_assert(sizeof(UnnamedEmptyBitfieldSplit) != (sizeof(short) * 2)); static_assert(!has_unique_object_representations<UnnamedEmptyBitfieldSplit>::value, "Bitfield padding"); |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
2822 | That was actually fine: the padding bits would not be a part of the unnamed bit field so they're detected. Then I realized that the surrounding code just sums the object representation bits and checks if that equals sizeof(T). But unnamed fields don't contribute any bits to the object representation. So a simpler way to fix this is to return 0 unconditionally. |
clang/test/SemaCXX/type-traits.cpp | ||
---|---|---|
2886–2889 | Do we also want to check packed structs as well? |
clang/test/SemaCXX/type-traits.cpp | ||
---|---|---|
2886–2889 | Do you have a test case where it would matter? Apparently packed is specified to ignore zero width bit fields. |
clang/test/SemaCXX/type-traits.cpp | ||
---|---|---|
2886–2889 | Apologies, I just realized you thought I meant another zero sized bit-field case whereas I meant just test packed in general w/ non-zero sized bit-fields. The result should match for unpacked but it would be good to verify considering we seem to always get bitten by cases we don't cover. |
I think the check for a non-zero bit-field width might not be completely correct in cases where it's used to split an allocation unit. I suggested a test case below.