Add support for referencable labels for attribute documentation #118428

ojhunt · 2024-12-03T05:31:05Z

The existing mechanism being used is to manually add a reference in the documentation. These references link to the beginning of the text rather than the heading for the attribute which is what this PR allows.

llvmbot · 2024-12-03T05:31:38Z

@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)

Changes

The existing mechanism being used is to manually add a reference in the documentation. These references link to the beginning of the text rather than the heading for the attribute which is what this PR allows.

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

3 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+3)
(modified) clang/include/clang/Basic/AttrDocs.td (+4-8)
(modified) clang/utils/TableGen/ClangAttrEmitter.cpp (+2)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 618252f3e75247..522821a79063ac 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -55,6 +55,9 @@ class Documentation {
   // When set, specifies that the attribute is deprecated and can optionally
   // specify a replacement attribute.
   DocDeprecated Deprecated;
+
+  // When set, specifies a label that can be used to reference the documentation
+  string Label = "";
 }
 
 // Specifies that the attribute is explicitly omitted from the documentation,
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 5de39be4805600..7a82b8fa320590 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -3360,9 +3360,8 @@ def NoSanitizeAddressDocs : Documentation {
   // This function has multiple distinct spellings, and so it requires a custom
   // heading to be specified. The most common spelling is sufficient.
   let Heading = "no_sanitize_address, no_address_safety_analysis";
+  let Label = "langext-address_sanitizer";
   let Content = [{
-.. _langext-address_sanitizer:
-
 Use ``__attribute__((no_sanitize_address))`` on a function or a global
 variable declaration to specify that address safety instrumentation
 (e.g. AddressSanitizer) should not be applied.
@@ -3372,9 +3371,8 @@ variable declaration to specify that address safety instrumentation
 def NoSanitizeThreadDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "no_sanitize_thread";
+  let Label = "langext-thread_sanitizer";
   let Content = [{
-.. _langext-thread_sanitizer:
-
 Use ``__attribute__((no_sanitize_thread))`` on a function declaration to
 specify that checks for data races on plain (non-atomic) memory accesses should
 not be inserted by ThreadSanitizer. The function is still instrumented by the
@@ -3385,9 +3383,8 @@ tool to avoid false positives and provide meaningful stack traces.
 def NoSanitizeMemoryDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "no_sanitize_memory";
+  let Label = "langext-memory_sanitizer";
   let Content = [{
-.. _langext-memory_sanitizer:
-
 Use ``__attribute__((no_sanitize_memory))`` on a function declaration to
 specify that checks for uninitialized memory should not be inserted
 (e.g. by MemorySanitizer). The function may still be instrumented by the tool
@@ -3398,9 +3395,8 @@ to avoid false positives in other places.
 def CFICanonicalJumpTableDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "cfi_canonical_jump_table";
+  let Label = "langext-cfi_canonical_jump_table";
   let Content = [{
-.. _langext-cfi_canonical_jump_table:
-
 Use ``__attribute__((cfi_canonical_jump_table))`` on a function declaration to
 make the function's CFI jump table canonical. See :ref:`the CFI documentation
 <cfi-canonical-jump-tables>` for more details.
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
index 534bf2d01d7957..a66868ba2cc044 100644
--- a/clang/utils/TableGen/ClangAttrEmitter.cpp
+++ b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -5178,6 +5178,8 @@ GetAttributeHeadingAndSpellings(const Record &Documentation,
 
 static void WriteDocumentation(const RecordKeeper &Records,
                                const DocumentationData &Doc, raw_ostream &OS) {
+  if (const StringRef label = Doc.Documentation->getValueAsString("Label"); !label.empty())
+    OS << ".. _" << label << ":\n\n";
   OS << Doc.Heading << "\n" << std::string(Doc.Heading.length(), '-') << "\n";
 
   // List what spelling syntaxes the attribute supports.

The existing mechanism being used is to manually add a reference in the documentation. These references link to the beginning of the text rather than the heading for the attribute which is what this PR allows.

Sirraide

Implementation seems fine to me; I can’t speak as to whether this is a necessary addition though.

Sirraide · 2024-12-03T06:08:47Z

clang/utils/TableGen/ClangAttrEmitter.cpp


 static void WriteDocumentation(const RecordKeeper &Records,
                               const DocumentationData &Doc, raw_ostream &OS) {
+  if (const StringRef label = Doc.Documentation->getValueAsString("Label");


Suggested change

if (const StringRef label = Doc.Documentation->getValueAsString("Label");

if (StringRef label = Doc.Documentation->getValueAsString("Label");

Also: label -> Label per our usual naming rules.

erichkeane · 2024-12-03T14:31:06Z

I have no idea what is going on here... @AaronBallman probably needs to take a better look. Else, could the author please give me an ELI5 sorta thing here or show me what the change looks like on the generated stuff?

AaronBallman

Out of curiosity, why does the existing mechanism not suffice?

AaronBallman · 2024-12-03T18:20:22Z

clang/include/clang/Basic/Attr.td

  // specify a replacement attribute.
  DocDeprecated Deprecated;
+
+  // When set, specifies a label that can be used to reference the documentation


Suggested change

// When set, specifies a label that can be used to reference the documentation

// When set, specifies a label that can be used to reference the documentation.

AaronBallman · 2024-12-03T18:21:06Z

clang/utils/TableGen/ClangAttrEmitter.cpp


 static void WriteDocumentation(const RecordKeeper &Records,
                               const DocumentationData &Doc, raw_ostream &OS) {
+  if (const StringRef label = Doc.Documentation->getValueAsString("Label");


Also: label -> Label per our usual naming rules.

ojhunt · 2024-12-04T04:25:11Z

Out of curiosity, why does the existing mechanism not suffice?

it "works" but creates the tag at the beginning of the text rather than the beginning of the attribute documentation. The result is that the scroll target for linking is the beginning of the text rather than the documentation as well, e.g. the location pointed to here:

As a result when you follow the link the actual attribute name, syntax table, etc is not visible and it is necessary to scroll to see the actual documentation being linked.

ojhunt · 2024-12-04T05:22:28Z

I have no idea what is going on here... @AaronBallman probably needs to take a better look. Else, could the author please give me an ELI5 sorta thing here or show me what the change looks like on the generated stuff?

Sure! This example will cover the docs used in the above screenshot

So currently the documentation for the attribute is:

def NoSanitizeMemoryDocs : Documentation {
  let Category = DocCatFunction;
  let Heading = "no_sanitize_memory";
  let Content = [{
.. _langext-memory_sanitizer:

Use ``__attribute__((no_sanitize_memory))`` on a function declaration to
....
  }];
}

this results in generated docs like this:

no_sanitize_memory
------------------
.. csv-table:: Supported Syntaxes
   :header: "GNU", "C++11", "C23", "``__declspec``", "Keyword", "``#pragma``", "HLSL Annotation", "``#pragma clang attribute``"

   "``no_address_safety_analysis`` |br| ``no_sanitize_address`` |br| ``no_sanitize_thread`` |br| ``no_sanitize_memory``","``gnu::no_address_safety_analysis`` |br| ``gnu::no_sanitize_address`` |br| ``gnu::no_sanitize_thread`` |br| ``clang::no_sanitize_memory``","``gnu::no_address_safety_analysis`` |br| ``gnu::no_sanitize_address`` |br| ``gnu::no_sanitize_thread`` |br| ``clang::no_sanitize_memory``","","","","","Yes"

.. _langext-memory_sanitizer: <!-- NOTE: This is where the manually specified label is -->

Use ``__attribute__((no_sanitize_memory))`` on a function declaration to
...

The result is that a :ref:langext-memory_sanitizer link goes into the middle of the attribute documentation, after the name and spelling table.

This change simply adds a Label field to the root Documentation object and updates the codgen to use it, and removes the currently manually specified labels, after which we can remove the manual label from the doc string above and use the explicit Label field which then results in this generated documentation:

.. _langext-memory_sanitizer: <!-- NOTE: This is where the manually specified label is now -->

no_sanitize_memory
------------------
.. csv-table:: Supported Syntaxes
   :header: "GNU", "C++11", "C23", "``__declspec``", "Keyword", "``#pragma``", "HLSL Annotation", "``#pragma clang attribute``"

   "``no_address_safety_analysis`` |br| ``no_sanitize_address`` |br| ``no_sanitize_thread`` |br| ``no_sanitize_memory``","``gnu::no_address_safety_analysis`` |br| ``gnu::no_sanitize_address`` |br| ``gnu::no_sanitize_thread`` |br| ``clang::no_sanitize_memory``","``gnu::no_address_safety_analysis`` |br| ``gnu::no_sanitize_address`` |br| ``gnu::no_sanitize_thread`` |br| ``clang::no_sanitize_memory``","","","","","Yes"

Use ``__attribute__((no_sanitize_memory))`` on a function declaration to
...

This has no impact on the visual appearance of the resultant html documentation, but places the anchor in a much better location for cross linking documentation.

Sirraide · 2024-12-04T07:38:43Z

This has no impact on the visual appearance of the resultant html documentation, but places the anchor in a much better location for cross linking documentation.

I see, I did’t quite pick up on that part because I don’t know RST very well, but I feel like that’s a reasonable addition then.

Sirraide

LGTM after fixing one more thing.

I personally think this is a reasonable addition now, but please wait for Aaron or Erich to approve this too before merging.

clang/utils/TableGen/ClangAttrEmitter.cpp

Sirraide · 2024-12-04T07:42:58Z

Also, if/when this is merged, it would be nice if someone could go through the rest of the docs and apply this wherever possible (assuming there are any other places where we still need to do that).

ojhunt · 2024-12-04T08:13:46Z

Also, if/when this is merged, it would be nice if someone could go through the rest of the docs and apply this wherever possible (assuming there are any other places where we still need to do that).

yeah, I wanted to isolate this for now to just the attribute docs as these are the only cases I could find, and I found them while working on the docs for #117428.

I'm kind of surprised I couldn't find other labels anywhere, but it could just be because other docs don't reference each other due to the inability to easily do so?

Co-authored-by: Sirraide <[email protected]>

ojhunt · 2024-12-04T08:16:13Z

LGTM after fixing one more thing.

I personally think this is a reasonable addition now, but please wait for Aaron or Erich to approve this too before merging.

Righto, I've committed the change, is const StringRef a style violation (I recognize StringRef is a constant type)? (Pondering adding support for style rule enforcement for things like "this is a constant type so doesn't need to be const")

Sirraide · 2024-12-04T08:46:28Z

is const StringRef a style violation

Yeah, we generally don’t use top-level const for variables that are value types (so it’s not because it’s a StringRef; it would be the same for e.g. const int x = foo()).

AaronBallman

Ah thank you for the more detailed explanation, this makes sense to me. LG!

erichkeane · 2024-12-04T15:04:23Z

Yeah, agreed, thanks for the ELI5. When it comes to RST/HTML, I'm roughly 5 :) LGTM as well.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 3, 2024

Add support for referencable labels for attribute documentation

aea2b40

The existing mechanism being used is to manually add a reference in the documentation. These references link to the beginning of the text rather than the heading for the attribute which is what this PR allows.

ojhunt force-pushed the users/ojhunt/attr-doc-label branch from 35fb507 to aea2b40 Compare December 3, 2024 05:32

Sirraide reviewed Dec 3, 2024

View reviewed changes

Sirraide requested review from AaronBallman and erichkeane December 3, 2024 06:12

Merge branch 'main' into users/ojhunt/attr-doc-label

661bf10

AaronBallman reviewed Dec 3, 2024

View reviewed changes

Adjust style

3f05ad0

Sirraide approved these changes Dec 4, 2024

View reviewed changes

clang/utils/TableGen/ClangAttrEmitter.cpp Outdated Show resolved Hide resolved

Sirraide requested a review from AaronBallman December 4, 2024 07:41

Update clang/utils/TableGen/ClangAttrEmitter.cpp

8ed1f24

Co-authored-by: Sirraide <[email protected]>

AaronBallman approved these changes Dec 4, 2024

View reviewed changes

Merge branch 'main' into users/ojhunt/attr-doc-label

31ffcbc

AaronBallman merged commit fe4bba6 into llvm:main Dec 4, 2024
9 checks passed

	if (const StringRef label = Doc.Documentation->getValueAsString("Label");
	if (StringRef label = Doc.Documentation->getValueAsString("Label");

	// When set, specifies a label that can be used to reference the documentation
	// When set, specifies a label that can be used to reference the documentation.

Add support for referencable labels for attribute documentation #118428

Add support for referencable labels for attribute documentation #118428

Uh oh!

Conversation

ojhunt commented Dec 3, 2024

Uh oh!

llvmbot commented Dec 3, 2024

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Sirraide Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Dec 3, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

ojhunt commented Dec 4, 2024

Uh oh!

ojhunt commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Dec 4, 2024

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sirraide commented Dec 4, 2024

Uh oh!

ojhunt commented Dec 4, 2024

Uh oh!

ojhunt commented Dec 4, 2024

Uh oh!

Sirraide commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Dec 4, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ojhunt commented Dec 4, 2024 •

edited

Loading

Sirraide commented Dec 4, 2024 •

edited

Loading