This is an archive of the discontinued LLVM Phabricator instance.

[Clang][AST] Fix __has_unique_object_representations computation for unnamed bitfields.
ClosedPublic

Authored by royjacobson on Mar 11 2023, 8:51 AM.

Details

Summary

As pointed out in https://github.com/llvm/llvm-project/issues/61336, objects with
unnamed bitfields aren't be uniquely representable.

Diff Detail

Event Timeline

royjacobson created this revision.Mar 11 2023, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 8:51 AM
royjacobson requested review of this revision.Mar 11 2023, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 8:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Change to a simpler fix.

royjacobson added subscribers: Restricted Project, foonathan.
Endill accepted this revision.Mar 11 2023, 9:50 AM

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).

This revision is now accepted and ready to land.Mar 11 2023, 9:50 AM

Handle 0-length unnamed bit fields as well.

aaron.ballman added inline comments.
clang/lib/AST/ASTContext.cpp
2728

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
2897–2900

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");

Add a test case, slightly simpler modeling.

royjacobson marked 2 inline comments as done.Mar 13 2023, 10:15 AM
royjacobson added inline comments.
clang/lib/AST/ASTContext.cpp
2728

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.

shafik added inline comments.Mar 13 2023, 9:25 PM
clang/test/SemaCXX/type-traits.cpp
2897–2900

Do we also want to check packed structs as well?

royjacobson marked an inline comment as done.Mar 15 2023, 1:25 PM
royjacobson added inline comments.
clang/test/SemaCXX/type-traits.cpp
2897–2900

Do you have a test case where it would matter? Apparently packed is specified to ignore zero width bit fields.

royjacobson added inline comments.Mar 23 2023, 3:06 PM
clang/test/SemaCXX/type-traits.cpp
2897–2900

small ping :) @shafik

shafik added inline comments.Mar 28 2023, 6:50 PM
clang/test/SemaCXX/type-traits.cpp
2897–2900

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.

Add some ((packed)) tests

shafik accepted this revision.Mar 30 2023, 2:18 PM

LGTM, thank for adding the tests!

This revision was landed with ongoing or failed builds.Apr 1 2023, 2:54 PM
This revision was automatically updated to reflect the committed changes.