[clangd] Show alignment for records and fields decls #67213

sr-tream · 2023-09-23T00:56:01Z

Shows align for records and fields declarations in hover information.

Example:

struct A {
  char a;
  short b;
};

For this struct hover informations shows:

Size: 4 bytes, alignment 2 bytes

llvmbot · 2023-09-23T00:57:12Z

@llvm/pr-subscribers-clangd

Changes

Shows align for records and fields declarations in hover information.

Example:

struct A {
  char a;
  short b;
};

For this struct hover informations shows:

Size: 4 bytes
Align: 2 bytes

Full diff: https://github.com/llvm/llvm-project/pull/67213.diff

3 Files Affected:

(modified) clang-tools-extra/clangd/Hover.cpp (+5)
(modified) clang-tools-extra/clangd/Hover.h (+2)
(modified) clang-tools-extra/clangd/unittests/HoverTests.cpp (+11)

diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 0ec85fc24df151b..f2cc6637069532b 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1001,6 +1001,8 @@ void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
   if (auto *RD = llvm::dyn_cast<RecordDecl>(&ND)) {
     if (auto Size = Ctx.getTypeSizeInCharsIfKnown(RD->getTypeForDecl()))
       HI.Size = Size->getQuantity() * 8;
+    if (!RD->isInvalidDecl() && !RD->isDependentType())
+      HI.Align = Ctx.getTypeAlign(RD->getTypeForDecl());
     return;
   }
 
@@ -1009,6 +1011,7 @@ void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
     if (Record)
       Record = Record->getDefinition();
     if (Record && !Record->isInvalidDecl() && !Record->isDependentType()) {
+      HI.Align = Ctx.getTypeAlign(FD->getType());
       const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(Record);
       HI.Offset = Layout.getFieldOffset(FD->getFieldIndex());
       if (FD->isBitField())
@@ -1488,6 +1491,8 @@ markup::Document HoverInfo::present() const {
           llvm::formatv(" (+{0} padding)", formatSize(*Padding)).str());
     }
   }
+  if (Align)
+    Output.addParagraph().appendText("Align: " + formatSize(*Align));
 
   if (CalleeArgInfo) {
     assert(CallPassType);
diff --git a/clang-tools-extra/clangd/Hover.h b/clang-tools-extra/clangd/Hover.h
index 6a61100912918ea..fe689de44732ebe 100644
--- a/clang-tools-extra/clangd/Hover.h
+++ b/clang-tools-extra/clangd/Hover.h
@@ -97,6 +97,8 @@ struct HoverInfo {
   std::optional<uint64_t> Offset;
   /// Contains the padding following a field within the enclosing class.
   std::optional<uint64_t> Padding;
+  /// Contains the alignment of fields and types where it's interesting.
+  std::optional<uint64_t> Align;
   // 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;
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 8c88cd52574536c..c5623759a7c2041 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -92,6 +92,7 @@ TEST(Hover, Structured) {
          HI.Offset = 0;
          HI.Size = 8;
          HI.Padding = 56;
+         HI.Align = 8;
          HI.AccessSpecifier = "private";
        }},
       // Union field
@@ -110,6 +111,7 @@ TEST(Hover, Structured) {
          HI.Type = "char";
          HI.Size = 8;
          HI.Padding = 120;
+         HI.Align = 8;
          HI.AccessSpecifier = "public";
        }},
       // Bitfield
@@ -128,6 +130,7 @@ TEST(Hover, Structured) {
          HI.Type = "int";
          HI.Offset = 0;
          HI.Size = 1;
+         HI.Align = 32;
          HI.AccessSpecifier = "public";
        }},
       // Local to class method.
@@ -192,6 +195,7 @@ TEST(Hover, Structured) {
          HI.Type = "char";
          HI.Offset = 0;
          HI.Size = 8;
+         HI.Align = 8;
          HI.AccessSpecifier = "public";
        }},
       // Struct definition shows size.
@@ -204,6 +208,7 @@ TEST(Hover, Structured) {
          HI.Kind = index::SymbolKind::Struct;
          HI.Definition = "struct X {}";
          HI.Size = 8;
+         HI.Align = 8;
        }},
       // Variable with template type
       {R"cpp(
@@ -1375,6 +1380,7 @@ class Foo final {})cpp";
          HI.Offset = 8;
          HI.Size = 1;
          HI.Padding = 23;
+         HI.Align = 8;
          HI.AccessSpecifier = "public";
        }}};
   for (const auto &Case : Cases) {
@@ -1411,6 +1417,7 @@ class Foo final {})cpp";
     EXPECT_EQ(H->Value, Expected.Value);
     EXPECT_EQ(H->Size, Expected.Size);
     EXPECT_EQ(H->Offset, Expected.Offset);
+    EXPECT_EQ(H->Align, Expected.Align);
     EXPECT_EQ(H->AccessSpecifier, Expected.AccessSpecifier);
     EXPECT_EQ(H->CalleeArgInfo, Expected.CalleeArgInfo);
     EXPECT_EQ(H->CallPassType, Expected.CallPassType);
@@ -3448,6 +3455,7 @@ template <typename T, typename C = bool> class Foo {})",
             HI.Size = 32;
             HI.Offset = 96;
             HI.Padding = 32;
+            HI.Align = 32;
           },
           R"(field foo
 
@@ -3455,6 +3463,7 @@ Type: type (aka can_type)
 Value = value
 Offset: 12 bytes
 Size: 4 bytes (+4 bytes padding)
+Align: 4 bytes
 
 // In test::Bar
 def)",
@@ -3470,6 +3479,7 @@ def)",
             HI.Size = 25;
             HI.Offset = 35;
             HI.Padding = 4;
+            HI.Align = 64;
           },
           R"(field foo
 
@@ -3477,6 +3487,7 @@ Type: type (aka can_type)
 Value = value
 Offset: 4 bytes and 3 bits
 Size: 25 bits (+4 bits padding)
+Align: 8 bytes
 
 // In test::Bar
 def)",

zyn0217

Thanks! This looks nice, but I think it's better to have more opinions from others.

Moreover, I have a question regarding the test: It seems the alignment for a field depends on the ABI spec, i.e., it could be different across MS ABI and Itanium ABI. Is that number stable for fundamental (or POD/trivial) type on most platforms? (I'm not an expert on ABI, so please correct me if I'm wrong.)

zyn0217 · 2023-09-23T09:53:13Z

clang-tools-extra/clangd/Hover.cpp

@@ -1001,6 +1001,8 @@ void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
  if (auto *RD = llvm::dyn_cast<RecordDecl>(&ND)) {
    if (auto Size = Ctx.getTypeSizeInCharsIfKnown(RD->getTypeForDecl()))
      HI.Size = Size->getQuantity() * 8;
+    if (!RD->isInvalidDecl() && !RD->isDependentType())


We already have a check for invalid decl at the beginning of the function: if (ND.isInvalidDecl()) return;; Maybe we could avoid this extra check? i.e., if (!RD->isDependentType()) is enough.

Yes, I removed this extra check in force-push

sr-tream · 2023-09-23T10:44:38Z

I'm not an expert on ABI

I too. I use exists function ASTContext::getTypeAlign - it must work correctly, if other functions like ASTContext::getTypeSizeInCharsIfKnown return correct results, because ASTContext::getTypeSizeInCharsIfKnown returns size with paddings for align

sr-tream · 2023-10-10T22:53:31Z

In force-push fixed clangd crash when hovered forward-declared types

Endilll

LGTM, but you should wait for other reviewers before merging.

Endilll · 2023-10-21T15:14:57Z

clang-tools-extra/clangd/Hover.cpp

@@ -1488,6 +1491,8 @@ markup::Document HoverInfo::present() const {
          llvm::formatv(" (+{0} padding)", formatSize(*Padding)).str());
    }
  }
+  if (Align)
+    Output.addParagraph().appendText("Align: " + formatSize(*Align));


My preference would be to have alignment on the same line as size, so that less vertical space is wasted. The following hover should illustrate why this might be more important than it appears on other hovers posted in this PR:

My preference would be to have alignment on the same line as size

But size may include padding for member fields. How would you like to see it?

Like this?

Size 24 bytes (8 bytes align) // for structs/classes Size 1 byte (+7 bytes padding, 8 bytes align) // for member fields with padding

That would work, but I'd prefer Size 24 bytes, alignment 8 bytes wording. I don't think alignment as a property of type is secondary to size to put it in parentheses.

Yeah, it's looks better

Endilll · 2023-10-21T15:29:03Z

On the topic of ABI, Itanium and Microsoft type layouts are not compatible, even for standard-layout (POD) types. For example, bit-fields are allowed in standard-layout types, but only Microsoft ABI mandates that bit-fields of different types can't be combined, and have to be padded. That's why almost all bit-fields in Clang are unsigned, without regards to types of values they hold. So author did the right thing asking Clang for alignment of types.

Triviality is another aspect of what has been known as POD, and has nothing to do with layouts: http://eel.is/c++draft/class.prop#2

sr-tream · 2023-10-21T18:29:29Z

In force-push changed alignment render, to render it in same line with size

zyn0217 · 2023-10-22T11:04:17Z

Thank you @Endilll for the explanation. The remaining part looks good; @sr-tream Do you have access to land the PR? If not, I'm glad to help you. :)

sr-tream · 2023-10-22T15:38:32Z

Do you have access to land the PR

No, help me please to merge this PR

llvmbot added the clangd label Sep 23, 2023

sr-tream mentioned this pull request Sep 23, 2023

Add alignment and padding to type hover info clangd/clangd#1763

Open

zyn0217 reviewed Sep 23, 2023

View reviewed changes

zyn0217 requested a review from HighCommander4 September 23, 2023 10:38

Endilll approved these changes Oct 21, 2023

View reviewed changes

Endilll added the enhancement Improving things as opposed to bug fixing, e.g. new or missing feature label Oct 21, 2023

[clangd] Show alignment for records and fields decls

b9b5bc2

Endilll merged commit c300884 into llvm:main Oct 22, 2023

sr-tream deleted the hover-align branch October 22, 2023 17:18

spupyrev mentioned this pull request Oct 23, 2023

[BOLT] Rename cds to cdsort #69966

Merged

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clangd] Show alignment for records and fields decls #67213

[clangd] Show alignment for records and fields decls #67213

Uh oh!

sr-tream commented Sep 23, 2023 •

edited

Loading

Uh oh!

llvmbot commented Sep 23, 2023

Uh oh!

zyn0217 left a comment •

edited

Loading

Uh oh!

zyn0217 Sep 23, 2023

Uh oh!

sr-tream Sep 23, 2023

Uh oh!

sr-tream commented Sep 23, 2023

Uh oh!

sr-tream commented Oct 10, 2023

Uh oh!

Endilll left a comment

Uh oh!

Endilll Oct 21, 2023

Uh oh!

sr-tream Oct 21, 2023

Uh oh!

Endilll Oct 21, 2023

Uh oh!

sr-tream Oct 21, 2023

Uh oh!

Endilll commented Oct 21, 2023

Uh oh!

sr-tream commented Oct 21, 2023

Uh oh!

zyn0217 commented Oct 22, 2023

Uh oh!

sr-tream commented Oct 22, 2023

Uh oh!

Uh oh!

[clangd] Show alignment for records and fields decls #67213

[clangd] Show alignment for records and fields decls #67213

Uh oh!

Conversation

sr-tream commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 23, 2023

Uh oh!

zyn0217 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

sr-tream Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

sr-tream commented Sep 23, 2023

Uh oh!

sr-tream commented Oct 10, 2023

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

Endilll Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

sr-tream Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

Endilll Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

sr-tream Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

Endilll commented Oct 21, 2023

Uh oh!

sr-tream commented Oct 21, 2023

Uh oh!

zyn0217 commented Oct 22, 2023

Uh oh!

sr-tream commented Oct 22, 2023

Uh oh!

Uh oh!

sr-tream commented Sep 23, 2023 •

edited

Loading

zyn0217 left a comment •

edited

Loading