[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

1467

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)

1474

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(...) { ... }

1478

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

struct HoverInfo {
   /// Set for functions and lambdas.
   std::optional<PrintedType> ReturnType;
   /// Set for functions, lambdas and macros with parameters.
   std::optional<std::vector<Param>> Parameters;
   /// Set for all templates(function, class, variable).
   std::optional<std::vector<Param>> TemplateParameters;
   /// Contains the evaluated value of the symbol if available.
   std::optional<std::string> Value;
-  /// Contains the byte-size of fields and types where it's interesting.
+  /// Contains the bit-size of fields and types where it's interesting.
   std::optional<uint64_t> Size;
   /// Contains the offset of fields within the enclosing class.
   std::optional<uint64_t> Offset;
   /// Contains the padding following a field within the enclosing class.
   std::optional<uint64_t> Padding;
   // Set when symbol is inside function call. Contains information extracted
   // from the callee definition about the argument this is passed as.
   std::optional<Param> CalleeArgInfo;

clang-tools-extra/clangd/Hover.cpp

HoverInfo getDeducedTypeHoverContents(QualType QT, const syntax::Token &Tok,
   return HI;
 }
 
 HoverInfo getStringLiteralContents(const StringLiteral *SL,
                                    const PrintingPolicy &PP) {
   HoverInfo HI;
 
   HI.Name = "string-literal";
-  HI.Size = (SL->getLength() + 1) * SL->getCharByteWidth();
+  HI.Size = (SL->getLength() + 1) * SL->getCharByteWidth() * 8;
   HI.Type = SL->getType().getAsString(PP).c_str();
 
   return HI;
 }
 
 bool isLiteral(const Expr *E) {
   // Unfortunately there's no common base Literal classes inherits from
   // (apart from Expr), therefore these exclusions.

 
 void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
   if (ND.isInvalidDecl())
     return;
 
   const auto &Ctx = ND.getASTContext();
   if (auto *RD = llvm::dyn_cast<RecordDecl>(&ND)) {
     if (auto Size = Ctx.getTypeSizeInCharsIfKnown(RD->getTypeForDecl()))
-      HI.Size = Size->getQuantity();
+      HI.Size = Size->getQuantity() * 8;
     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())) {
-        HI.Size = FD->isZeroSize(Ctx) ? 0 : Size->getQuantity();
+      HI.Offset = Layout.getFieldOffset(FD->getFieldIndex());
+      if (FD->isBitField())
+        HI.Size = FD->getBitWidthValue(Ctx);
+      else if (auto Size = Ctx.getTypeSizeInCharsIfKnown(FD->getType()))
+        HI.Size = FD->isZeroSize(Ctx) ? 0 : Size->getQuantity() * 8;
+      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;
+          unsigned NextOffset = Layout.getFieldOffset(FD->getFieldIndex() + 1);
           if (NextOffset >= EndOfField) // next field could be a bitfield!
             HI.Padding = NextOffset - EndOfField;
         } else {
           // Measure padding up to the end of the object.
-          HI.Padding = Layout.getSize().getQuantity() - EndOfField;
+          HI.Padding = Layout.getSize().getQuantity() * 8 - EndOfField;
         }
       }
       // Offset in a union is always zero, so not really useful to report.
       if (Record->isUnion())
         HI.Offset.reset();
     }
     return;
   }
Output.addParagraph().appendText("Type: ").appendCode(
         llvm::to_string(*Type));
 
   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());
+  const auto FormatSize = [](uint64_t Bits) {
+    uint64_t Value = Bits % 8 == 0 ? Bits / 8 : Bits;
+    const char *Unit = Value != 0 && Value == Bits ? "bit" : "byte";
+    return llvm::formatv("{0} {1}{2}", Value, Unit, Value == 1 ? "" : "s")
+        .str();
+  };
+
+  if (Offset) {
+    const auto Bytes = *Offset / 8;
+    const auto Bits = *Offset % 8;
+    auto &P = Output.addParagraph().appendText("Offset: ");
+    if (Bytes != 0 || Bits == 0)
+      P.appendText(FormatSize(Bytes * 8));
+    if (Bits != 0) {
+      if (Bytes != 0)
+        P.appendText(" and ");
+      P.appendText(FormatSize(Bits));
+    }
+  }
   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());
+    auto &P = Output.addParagraph().appendText("Size: ");
+    P.appendText(FormatSize(*Size));
+    if (Padding && *Padding != 0) {
+      P.appendText(
+          llvm::formatv(" (+{0} padding)", FormatSize(*Padding)).str());
+    }
   }
 
   if (CalleeArgInfo) {
     assert(CallPassType);
     std::string Buffer;
     llvm::raw_string_ostream OS(Buffer);
     OS << "Passed ";
     if (CallPassType->PassBy != HoverInfo::PassType::Value) {

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

const std::function<void(HoverInfo &)> ExpectedBuilder;
        [](HoverInfo &HI) {
          HI.NamespaceScope = "ns1::ns2::";
          HI.LocalScope = "Foo::";
          HI.Name = "bar";
          HI.Kind = index::SymbolKind::Field;
          HI.Definition = "char bar";
          HI.Type = "char";
          HI.Offset = 0;
-         HI.Size = 1;
-         HI.Padding = 7;
+         HI.Size = 8;
+         HI.Padding = 56;
          HI.AccessSpecifier = "private";
        }},
       // Union field
       {R"cpp(
             union Foo {
               char [[b^ar]];
               double y[2];
             };
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.LocalScope = "Foo::";
          HI.Name = "bar";
          HI.Kind = index::SymbolKind::Field;
          HI.Definition = "char bar";
          HI.Type = "char";
-         HI.Size = 1;
-         HI.Padding = 15;
+         HI.Size = 8;
+         HI.Padding = 120;
          HI.AccessSpecifier = "public";
        }},
       // Bitfield
       {R"cpp(
             struct Foo {
               int [[^x]] : 1;
               int y : 1;
             };
           )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]];
const std::function<void(HoverInfo &)> ExpectedBuilder;
        [](HoverInfo &HI) {
          HI.NamespaceScope = "ns1::";
          HI.LocalScope = "(anonymous struct)::";
          HI.Name = "bar";
          HI.Kind = index::SymbolKind::Field;
          HI.Definition = "char bar";
          HI.Type = "char";
          HI.Offset = 0;
-         HI.Size = 1;
+         HI.Size = 8;
          HI.AccessSpecifier = "public";
        }},
       // Struct definition shows size.
       {R"cpp(
           struct [[^X]]{};
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.Name = "X";
          HI.Kind = index::SymbolKind::Struct;
          HI.Definition = "struct X {}";
-         HI.Size = 1;
+         HI.Size = 8;
        }},
       // Variable with template type
       {R"cpp(
           template <typename T, class... Ts> class Foo { public: Foo(int); };
           Foo<int, char, bool> [[fo^o]] = Foo<int, char, bool>(5);
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";

        [](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 = 8;
+         HI.Size = 1;
+         HI.Padding = 23;
+         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
const std::function<void(HoverInfo &)> ExpectedBuilder;
        [](HoverInfo &HI) {
          HI.Name = "expression";
          HI.Type = "char";
          HI.Value = "65 (0x41)";
        }},
       {"auto s = ^[[\"Hello, world!\"]]; // string literal",
        [](HoverInfo &HI) {
          HI.Name = "string-literal";
-         HI.Size = 14;
+         HI.Size = 112;
          HI.Type = "const char[14]";
        }},
       {
           R"cpp(// Local variable
             int main() {
               int bonjour;
               ^[[bonjour]] = 2;
               int test1 = bonjour;
llvm::StringRef ExpectedRender;
             HI.Kind = index::SymbolKind::NamespaceAlias;
             HI.Name = "foo";
           },
           R"(namespace-alias foo)",
       },
       {
           [](HoverInfo &HI) {
             HI.Kind = index::SymbolKind::Class;
-            HI.Size = 10;
+            HI.Size = 80;
             HI.TemplateParameters = {
                 {{"typename"}, std::string("T"), std::nullopt},
                 {{"typename"}, std::string("C"), std::string("bool")},
             };
             HI.Documentation = "documentation";
             HI.Definition =
                 "template <typename T, typename C = bool> class Foo {}";
             HI.Name = "foo";
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;
-            HI.Padding = 4;
+            HI.Size = 32;
+            HI.Offset = 96;
+            HI.Padding = 32;
           },
           R"(field foo
 
 Type: type (aka can_type)
 Value = value
 Offset: 12 bytes
-Size: 4 bytes (+4 padding)
+Size: 4 bytes (+4 bytes padding)
 
 // In test::Bar
 def)",
       },
       {
           [](HoverInfo &HI) {
             HI.Kind = index::SymbolKind::Field;
             HI.AccessSpecifier = "public";