Fix include order in CXType.cpp

[collinbaker]

Summary

Handle template parameter-dependent bit field widths in libclang

In a class template, a bit field's width may depend on a template
parameter. In this case the width expression cannot be evaluated.

Previously clang_getFieldDeclBitWidth() would assert, or cause memory
unsafety and return an invalid result if assertions are disabled.

This adds a check for this case which returns an error code. An
additional function clang_isBitFieldDecl() is added to disambiguate
between error code meanings.

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

[rnk]

Dmitri, do you know a good libclang point of contact for the new API?

[collinbaker]

Pinging this

[rnk]

Pinging alternative reviewer +@dexonsmith for a libclang API addition

[dexonsmith]

In D130303#3724247, @rnk wrote:

Pinging alternative reviewer +@dexonsmith for a libclang API addition

Looks reasonable to me -- this only changes behaviour of the existing API when there was corruption before -- but if the goal is to get a vendor of libclang-as-a-stable-API to sign off, I can't help.

@arphaman, if you're busy, is there someone else that could take a quick look?

[vedgy]

I just verified that this patch fixes a KDevelop crash reported by several users. Thank you!

@aaron.ballman, could you please review this fix?

clang/include/clang-c/Index.h

3032

New API has to be mentioned in the libclang section of clang/docs/ReleaseNotes.rst and in the LLVM_17 block of clang/tools/libclang/libclang.map.

clang/tools/libclang/CXType.cpp

395

I thought of the same fix while analyzing the assertion failure here: https://bugs.kde.org/show_bug.cgi?id=438249#c19

An alternative implementation is to place this same check in clang::FieldDecl::getBitWidthValue(). This looks like a general issue that could affect non-libclang users of clang::FieldDecl::getBitWidthValue(). But apparently no one else has stumbled upon it.

clang::FieldDecl::getBitWidthValue() returns unsigned and has no error-reporting mechanism yet. It could return std::numeric_limits<unsigned>::max() (or 0 if that's an invalid bit width value) in case of the value dependence.

[vedgy]

clang/include/clang-c/Index.h

3037

Append the following line to the commit message to properly reference and (hopefully) close the bug automatically:

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

clang/tools/libclang/CXType.cpp

18

Since the code worked fine before, I don't think this include should be added.

[vedgy]

clang/include/clang-c/Index.h

3030

I just thought how the new API could be used in KDevelop. Currently when clang_getFieldDeclBitWidth() is positive, e.g. 2, KDevelop shows : 2 after the data member name in a tooltip. Ideally a template-param-dependent expression (actual code) would be displayed after the colon. If that's difficult to implement, : [tparam-dependent] or : ? could be displayed instead. But it would be more convenient and efficient to get this information by a single call to clang_getFieldDeclBitWidth() instead of calling clang_isFieldDeclBitWidthDependent() each time clang_getFieldDeclBitWidth() returns -1. So how about returning -2 or 0 from clang_getFieldDeclBitWidth() instead of adding this new API?

[dexonsmith]

In D130303#3724392, @dexonsmith wrote:

In D130303#3724247, @rnk wrote:

Pinging alternative reviewer +@dexonsmith for a libclang API addition

Looks reasonable to me -- this only changes behaviour of the existing API when there was corruption before -- but if the goal is to get a vendor of libclang-as-a-stable-API to sign off, I can't help.

@arphaman, if you're busy, is there someone else that could take a quick look?

This still LGTM, for the same reasons. Let's just ship this.

[collinbaker]

Sync with upstream changes

[collinbaker]

Add release notes and remove unneeded include

clang/include/clang-c/Index.h

3030

I understand the motivation but I don't think requiring an extra call is asking too much of libclang clients, and it's one extra call that doesn't do much work and will be called rarely so I don't see efficiency concerns. Without strong reasons otherwise I think it's better to be explicit here.

[collinbaker]

Add symbol to version map

[collinbaker]

Thanks for the review. Someone else will need to commit since I don't have permission.

[vedgy]

clang/include/clang-c/Index.h

3030

KDevelop calls clang_getFieldDeclBitWidth() for each encountered class member declaration. clang_isFieldDeclBitWidthDependent() would have to be called each time clang_getFieldDeclBitWidth() returns -1, which would be most of the time, because few class members are bit-fields. The work this new function does is the same as that of clang_getFieldDeclBitWidth() (repeated).

If the concern about returning -2 from clang_getFieldDeclBitWidth() is cryptic return codes, an enum with named constants can be introduced.

If the concern is breaking backward compatibility for users that relied on the returned value being positive or -1, then a replacement for clang_getFieldDeclBitWidth() with the most convenient API should be introduced and clang_getFieldDeclBitWidth() itself - deprecated.

KDevelop simply stores the value returned by clang_getFieldDeclBitWidth() in an int16_t m_bitWidth data member and uses it later. So if -2 is returned, the only place in code to adjust would be the use of this data member. With the current clang_isFieldDeclBitWidthDependent() implementation, this function would have to be called, -2 explicitly stored in m_bitWidth and the use of m_bitWidth would have to be adjusted just the same.

Have you considered potential usage of the added API in your project? Which alternative would be more convenient to use?

clang/tools/libclang/CXType.cpp

13

I guess clang-format did this include reordering. But it certainly looks out of place and the include order becomes wrong. So I think it should be reverted.

[vedgy]

clang/include/clang-c/Index.h

3030

One more API alternative is to replace clang_isFieldDeclBitWidthDependent() with clang_isBitFieldDecl(). The usage would be more straightforward and efficient: first call clang_isBitFieldDecl(); if it returns true (should be rare enough), call clang_getFieldDeclBitWidth(); if that returns -1, then the bit-field width must be unknown (dependent on template parameters). Such usage would still be less convenient and efficient compared to clang_getFieldDeclBitWidth() returning -2 though.

[collinbaker]

@dexonsmith can you weigh in?

[dexonsmith]

In D130303#4175664, @collinbaker wrote:

@dexonsmith can you weigh in?

Introducing clang_isBitFieldDecl sounds like a clean/straightforward solution to me.

[aaron.ballman]

In D130303#4176330, @dexonsmith wrote:

In D130303#4175664, @collinbaker wrote:

@dexonsmith can you weigh in?

Introducing clang_isBitFieldDecl sounds like a clean/straightforward solution to me.

+1, that seems less mysterious than returning -2 from clang_getFieldDeclBitWidth() to me.

[collinbaker]

Requested changes

[collinbaker]

Changed as requested. Again leaving it up to a committer to commit this

clang/include/clang-c/Index.h

3030

Implemented as clang_isBitFieldDecl rather than magic return value

clang/tools/libclang/CXType.cpp

13

I don't agree, it's pretty standard for a source file to have its associated header include at the top.

395

This would be suitable for a follow-up, since it doesn't affect the public interface of libclang.

[vedgy]

Please update the commit message (there is no clang_isFieldDeclBitWidthDependent anymore) and update the revision with arc diff --verbatim @~.

clang/include/clang-c/Index.h

3030

Thanks. That's good enough for me.

clang/tools/libclang/CXType.cpp

13

You are right, I haven't realized the header-source association. The diff is still unrelated to the patch. But I'm no longer sure what's right, so won't insist on anything.

[dexonsmith]

clang/tools/libclang/CXType.cpp

13

This is correct behaviour from clang-format.

Given that there were no functional changes to includes, typically we’d omit clang-format cleanups. (I know there was a change below originally, but that was reverted.)

I’m fine leaving the header clean-up in, or separating it out to different NFC commit (ideal; could be done when pushing), or skipping it entirely.

[aaron.ballman]

Found some minor cleanups but otherwise LGTM (feel free to fix when landing if you'd like).

clang/tools/libclang/CXType.cpp

13

+1 -- FWIW, we usually ask for formatting changes to be separated out into a separate commit because it makes git blame more useful when we're doing code archeology later, but this is minor enough to be unlikely to cause an issue, so I don't feel strongly.

392–395

The conversion is always going to do the right thing: https://eel.is/c++draft/conv#integral-2.sentence-2

407

[collinbaker]

Cleanups and split header order change to separate commit

[collinbaker]

Missed commit

[collinbaker]

Re-add "Fixes" commit message footer

[collinbaker]

I am very bad at using differential

[collinbaker]

Rebase w/ conflict in libclang.map

[collinbaker]

All done. Again, I don't have commit access so someone else will need to land this.

[aaron.ballman]

In D130303#4183081, @collinbaker wrote:

All done. Again, I don't have commit access so someone else will need to land this.

I can do that for you -- what name and email address would you like me to use for patch attribution?

[collinbaker]

Collin Baker
[e-mail]

This should also be in the author field of the commits

Thanks!

[vedgy]

clang/tools/libclang/CXType.cpp

374

I just noticed the clang_Cursor_isBitField() function implemented 10 years ago , which returns exactly the same value as this new function. So most changes in this commit can be reverted.

[vedgy]

clang/tools/libclang/CXType.cpp

374

clang_Cursor_isBitField() is declared much later in Index.h and isn't easily discoverable. clang_getFieldDeclBitWidth() could benefit from a usage example. Here is how I plan to use it in KDevelop:

if (clang_Cursor_isBitField(cursor)) {
    const auto bitWidth = clang_getFieldDeclBitWidth(cursor);
    decl->setBitWidth(bitWidth == -1 ? ClassMemberDeclaration::ValueDependentBitWidth : bitWidth);
}

[aaron.ballman]

clang/tools/libclang/CXType.cpp

374

I just noticed the clang_Cursor_isBitField() function implemented 10 years ago , which returns exactly the same value as this new function. So most changes in this commit can be reverted.

Wow, good catch!

I'm going to roll this commit back in its entirety rather than do a partial revert, then I'll push a new changes to add back the isValueDependent() bit below, and rearrange the header file + add an example.

[vedgy]

clang/tools/libclang/CXType.cpp

374

Sounds good: the minimal-fix patch can be easily cherry-picked to older libclang versions by distro maintainers. Thank you.

[aaron.ballman]

I reverted this patch in 20fb4c3270b9d3596f213623d39c507711d1c165 and I've put up a new patch for review at https://reviews.llvm.org/D146039.