This is an archive of the discontinued LLVM Phabricator instance.

[clang] Update NumFunctionDeclBits for FunctionDeclBitfields
ClosedPublic

Authored by danix800 on Aug 16 2023, 11:02 PM.

Details

Summary

FunctionDeclBitfields.NumFunctionDeclBits is not updated when DeductionCandidateKind is incremented.
CXXConstructorDeclBitfields.NumCtorInitializers is shrunk as needed.

Fixes https://github.com/llvm/llvm-project/issues/64171

Diff Detail

Event Timeline

danix800 created this revision.Aug 16 2023, 11:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
danix800 requested review of this revision.Aug 16 2023, 11:02 PM
danix800 edited the summary of this revision. (Show Details)Aug 16 2023, 11:08 PM
danix800 edited the summary of this revision. (Show Details)
danix800 updated this revision to Diff 551016.Aug 16 2023, 11:42 PM

Add alternative testcase.

For context this seems to be the patch that did not increment the bit field https://reviews.llvm.org/D139837

Thanks for working on this!
Looks good to me except for a couple typos.
Can you an an entry in clang/docs/ReleaseNotes.rst (mentioning the github issue)

clang/unittests/AST/ASTImporterTest.cpp
7836

ImportFunctionDeclBitShouldNotOverwriteCtorDeclBits

clang/unittests/AST/DeclTest.cpp
356 ↗(On Diff #551016)

Can you an an entry in clang/docs/ReleaseNotes.rst (mentioning the github issue)

No problem!

danix800 added inline comments.Aug 17 2023, 12:08 AM
clang/unittests/AST/ASTImporterTest.cpp
7836

I added the case into DeclTest.cpp because i think it can cover the root cause for both testcases.

How about remove the somewhat repeated one in ASTImporterTest.cpp?

I also investigated whether we could count those bits at compile time and statically assert on them,
because a small typo or missed update could spend us a lot of time to dig for the cause.

My first step is trying to count number of bits for a single bitfield, this is promising based on this but
with a restriction, it only works on struct (default public fields), not class (default to private fields).

If we can implement this bitsizeof then we could have:

enum { NumFunctionDeclBits = offsetof(FunctionDeclBitfields, SClass)
                           + offsetof(FunctionDeclBitfields, IsInline)
                           + ... };

This can automatically update total number of bits if any of the existing one is updated.

The second step is trying to enumerate all bit fields at compile time so that we can totally fix this kind
of issue, but it seems not possible.

Any suggestions or advices? Is it even worth it to do it like this?

I also investigated whether we could count those bits at compile time and statically assert on them,
because a small typo or missed update could spend us a lot of time to dig for the cause.

My first step is trying to count number of bits for a single bitfield, this is promising based on this but
with a restriction, it only works on struct (default public fields), not class (default to private fields).

If we can implement this bitsizeof then we could have:

enum { NumFunctionDeclBits = offsetof(FunctionDeclBitfields, SClass)
                           + offsetof(FunctionDeclBitfields, IsInline)
                           + ... };

This can automatically update total number of bits if any of the existing one is updated.

The second step is trying to enumerate all bit fields at compile time so that we can totally fix this kind
of issue, but it seems not possible.

Any suggestions or advices? Is it even worth it to do it like this?

It would be great to be able to stattic_assert these sort of bugs. I think @aaron did some research in this area, it might be worth asking him how far he got!

clang/unittests/AST/ASTImporterTest.cpp
7836

Now that you have written it, we can leave both tests i think.
Afaik one test serialization so they complement each other

danix800 added inline comments.Aug 17 2023, 1:00 AM
clang/unittests/AST/ASTImporterTest.cpp
7836

OK! :-)

danix800 updated this revision to Diff 551037.Aug 17 2023, 1:13 AM
  1. Update ReleaseNotes.rst
  2. Fix typos pointed out by @cor3ntin (thanks!)
danix800 marked 2 inline comments as done.Aug 17 2023, 1:14 AM
balazske added inline comments.Aug 17 2023, 2:20 AM
clang/unittests/AST/DeclTest.cpp
368 ↗(On Diff #551037)

This dump is not needed?

danix800 added inline comments.Aug 17 2023, 2:30 AM
clang/unittests/AST/DeclTest.cpp
368 ↗(On Diff #551037)

Yeah it's for debug only, I'll remove it.

danix800 updated this revision to Diff 551078.Aug 17 2023, 3:44 AM

Remove debug code.

danix800 marked an inline comment as done.Aug 17 2023, 3:44 AM
aaron.ballman accepted this revision.Aug 17 2023, 5:07 AM

I also investigated whether we could count those bits at compile time and statically assert on them,
because a small typo or missed update could spend us a lot of time to dig for the cause.

My first step is trying to count number of bits for a single bitfield, this is promising based on this but
with a restriction, it only works on struct (default public fields), not class (default to private fields).

If we can implement this bitsizeof then we could have:

enum { NumFunctionDeclBits = offsetof(FunctionDeclBitfields, SClass)
                           + offsetof(FunctionDeclBitfields, IsInline)
                           + ... };

This can automatically update total number of bits if any of the existing one is updated.

The second step is trying to enumerate all bit fields at compile time so that we can totally fix this kind
of issue, but it seems not possible.

Any suggestions or advices? Is it even worth it to do it like this?

It would be great to be able to stattic_assert these sort of bugs. I think @aaron did some research in this area, it might be worth asking him how far he got!

I was never able to find a conforming way to do it without implementing something like a "bitwidth of" operator, which is something I was planning to propose to WG14 anyway for use with _BitInt. I've not implemented the extension yet though, and even if I did, it wouldn't help us with non-Clang host compilers, so I eventually gave up trying to automatically calculate the values.

Changes LGTM!

This revision is now accepted and ready to land.Aug 17 2023, 5:07 AM
This revision was landed with ongoing or failed builds.Aug 17 2023, 5:30 AM
This revision was automatically updated to reflect the committed changes.