[clangd] Show size, offset and padding for bit fields on hover

[SR_team]

Summary

Examle:

struct test {
	char a;
	char b : 3;
	char c : 5;
	int d;
	int e : 27;
};

[sammccall]

I do see the appeal of making this work for bitfields, but I think silently rounding offset/size/padding to whole bytes is pretty misleading.
For bitfields we'd really need to talk about layout at a bit level.
But we don't want to talk about regular fields in bits (I think?!)
And having a mixture seems like it's going to be pretty complicated both in implementation and for users, for a smallish feature.

I don't see a great sweet-spot design, maybe you/others have ideas (or can explain why the rounding behavior in this patch is reasonable.

[SR_team]

In D151128#4365218, @sammccall wrote:

I do see the appeal of making this work for bitfields, but I think silently rounding offset/size/padding to whole bytes is pretty misleading.
For bitfields we'd really need to talk about layout at a bit level.
But we don't want to talk about regular fields in bits (I think?!)
And having a mixture seems like it's going to be pretty complicated both in implementation and for users, for a smallish feature.

I don't see a great sweet-spot design, maybe you/others have ideas (or can explain why the rounding behavior in this patch is reasonable.

We can store this fields in bits, and show in bytes on hover, when it's possible, and in bits, when showing in bytes impossible. Would such an implementation be good?

[SR_team]

This comment was removed by SR_team.

[SR_team]

Stores Offset, Size and Padding fields of HoverInfo in bits. When hover present bits converted to bytes, if possible.

Examle:

struct test {
	char a;
	char b : 3;
	char c : 5;
	int d;
	int e : 31;
};

[SR_team]

Extract formatting to lambda, for simplify code modification

[SR_team]

@sammccall review my code please

[sammccall]

Sorry for the delay, I've been out on vacation.

Behavior looks great and isn't too complicated :-)
Let me know if you have commit access or I should land this for you (in which case: please provide a preferred name/email for attribution)

clang-tools-extra/clangd/Hover.cpp

1472

can you make this a separate (static) function rather than a lambda?
and the same for formatOffset()

(This is a long function as it is)

1479

The distinction between bits+bytes for offsets vs bits OR bytes for size is worth calling out with a comment:

// Sizes (and padding) are shown in bytes if possible, otherwise in bits.
string formatSize(...) { ... }

// Offsets are shown in bytes + bits, so offsets of different fields
// can always be easily compared.
string formatOffset(...) { ... }

1483

FWIW I think I slightly prefer printing bytes always, so that 4 prints as 0 bytes and 4 bits.

This makes a rare case slightly more regular (and the code is simpler). Up to you though.

clang-tools-extra/clangd/unittests/HoverTests.cpp

3292

can you add a testcase for render() where the size/offset/padding are not whole bytes?

[SR_team]

• moved lambda FormatSize to static function formatSize();
• extracted offset format code to static function formatOffset();
• always shows bytes in offset (for offset 4 showed Offset: 0 bytes and 4 bits)
• added render test for bit-fields

Let me know if you have commit access or I should land this for you (in which case: please provide a preferred name/email for attribution)

Land it for me, please. Name: SR_team, email: [e-mail]

clang-tools-extra/clangd/Hover.cpp

if (auto Size = Ctx.getTypeSizeInCharsIfKnown(RD->getTypeForDecl()))
       HI.Size = Size->getQuantity();
     return;
   }
 
   if (const auto *FD = llvm::dyn_cast<FieldDecl>(&ND)) {
     const auto *Record = FD->getParent();
     if (Record)
       Record = Record->getDefinition();
-    if (Record && !Record->isInvalidDecl() && !Record->isDependentType() &&
-        !FD->isBitField()) {
+    if (Record && !Record->isInvalidDecl() && !Record->isDependentType()) {
       const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(Record);
       HI.Offset = Layout.getFieldOffset(FD->getFieldIndex()) / 8;
-      if (auto Size = Ctx.getTypeSizeInCharsIfKnown(FD->getType())) {
+      if (FD->isBitField()) {
+        const auto SizeInBits = FD->getBitWidthValue(Ctx);
+        HI.Size = (SizeInBits + 7) >> 3;
+      } else if (auto Size = Ctx.getTypeSizeInCharsIfKnown(FD->getType()))
         HI.Size = FD->isZeroSize(Ctx) ? 0 : Size->getQuantity();
+      if (HI.Size) {
         unsigned EndOfField = *HI.Offset + *HI.Size;
 
         // Calculate padding following the field.
         if (!Record->isUnion() &&
             FD->getFieldIndex() + 1 < Layout.getFieldCount()) {
           // Measure padding up to the next class field.
           unsigned NextOffset =
               Layout.getFieldOffset(FD->getFieldIndex() + 1) / 8;
if (Value) {
     markup::Paragraph &P = Output.addParagraph();
     P.appendText("Value = ");
     P.appendCode(*Value);
   }
 
   if (Offset)
     Output.addParagraph().appendText(
         llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s")
             .str());
   if (Size) {
     auto &P = Output.addParagraph().appendText(
         llvm::formatv("Size: {0} byte{1}", *Size, *Size == 1 ? "" : "s").str());
     if (Padding && *Padding != 0)
       P.appendText(llvm::formatv(" (+{0} padding)", *Padding).str());
   }
 
   if (CalleeArgInfo) {
     assert(CallPassType);
     std::string Buffer;
     llvm::raw_string_ostream OS(Buffer);
     OS << "Passed ";
     if (CallPassType->PassBy != HoverInfo::PassType::Value) {
       OS << "by ";
       if (CallPassType->PassBy == HoverInfo::PassType::ConstRef)
         OS << "const ";
       OS << "reference ";
     }
     if (CalleeArgInfo->Name)

clang-tools-extra/clangd/unittests/HoverTests.cpp

const std::function<void(HoverInfo &)> ExpectedBuilder;
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.LocalScope = "Foo::";
          HI.Name = "x";
          HI.Kind = index::SymbolKind::Field;
          HI.Definition = "int x : 1";
          HI.Type = "int";
+         HI.Offset = 0;
+         HI.Size = 1;
          HI.AccessSpecifier = "public";
        }},
       // Local to class method.
       {R"cpp(
           namespace ns1 { namespace ns2 {
             struct Foo {
               void foo() {
                 int [[b^ar]];

        [](HoverInfo &HI) {
          HI.Name = "arr";
          HI.NamespaceScope = "";
          HI.LocalScope = "Test<Size>::";
          HI.AccessSpecifier = "public";
          HI.Kind = index::SymbolKind::Field;
          HI.Definition = "m_int arr[Size]";
          HI.Type = {"m_int[Size]", "int[Size]"};
+       }},
+      {// Bitfield offset, size and padding
+       R"cpp(
+            struct Foo {
+              char x;
+              char [[^y]] : 1;
+              int z;
+            };
+          )cpp",
+       [](HoverInfo &HI) {
+         HI.NamespaceScope = "";
+         HI.LocalScope = "Foo::";
+         HI.Name = "y";
+         HI.Kind = index::SymbolKind::Field;
+         HI.Definition = "char y : 1";
+         HI.Type = "char";
+         HI.Offset = 1;
+         HI.Size = 1;
+         HI.Padding = 2;
+         HI.AccessSpecifier = "public";
        }}};
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Code);
 
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-std=c++17");
     // Types might be different depending on the target triplet, we chose a
template <typename T, typename C = bool> class Foo {})",
           "- type (aka can_type)\n"
           "- type foo (aka can_type)\n"
           "- type foo = default (aka can_type)\n"
           "\n"
           "// In namespace ns\n"
           "ret_type foo(params) {}",
       },
       {
           [](HoverInfo &HI) {
             HI.Kind = index::SymbolKind::Field;
             HI.LocalScope = "test::Bar::";
             HI.Value = "value";
             HI.Name = "foo";
             HI.Type = {"type", "can_type"};
             HI.Definition = "def";
             HI.Size = 4;
             HI.Offset = 12;