[LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper #107536

jurahul · 2024-09-06T07:40:07Z

Migrate LLDB TableGen backend to use const RecordKeeper.

This is a part of effort to have better const correctness in TableGen backends:

https://discourse.llvm.org/t/psa-planned-changes-to-tablegen-getallderiveddefinitions-api-potential-downstream-breakages/81089

Migrate LLDB TableGen backend to use const RecordKeeper.

llvmbot · 2024-09-06T08:22:51Z

@llvm/pr-subscribers-lldb

Author: Rahul Joshi (jurahul)

Changes

Migrate LLDB TableGen backend to use const RecordKeeper.

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

6 Files Affected:

(modified) lldb/utils/TableGen/LLDBOptionDefEmitter.cpp (+6-7)
(modified) lldb/utils/TableGen/LLDBPropertyDefEmitter.cpp (+11-10)
(modified) lldb/utils/TableGen/LLDBTableGen.cpp (+1-1)
(modified) lldb/utils/TableGen/LLDBTableGenBackends.h (+3-3)
(modified) lldb/utils/TableGen/LLDBTableGenUtils.cpp (+2-2)
(modified) lldb/utils/TableGen/LLDBTableGenUtils.h (+3-2)

diff --git a/lldb/utils/TableGen/LLDBOptionDefEmitter.cpp b/lldb/utils/TableGen/LLDBOptionDefEmitter.cpp
index b48a0e4beda3a9..735489c0e56a41 100644
--- a/lldb/utils/TableGen/LLDBOptionDefEmitter.cpp
+++ b/lldb/utils/TableGen/LLDBOptionDefEmitter.cpp
@@ -35,7 +35,7 @@ struct CommandOption {
   std::string Description;
 
   CommandOption() = default;
-  CommandOption(Record *Option) {
+  CommandOption(const Record *Option) {
     if (Option->getValue("Groups")) {
       // The user specified a list of groups.
       auto Groups = Option->getValueAsListOfInts("Groups");
@@ -145,11 +145,9 @@ static void emitOption(const CommandOption &O, raw_ostream &OS) {
 }
 
 /// Emits all option initializers to the raw_ostream.
-static void emitOptions(std::string Command, std::vector<Record *> Records,
+static void emitOptions(std::string Command, ArrayRef<const Record *> Records,
                         raw_ostream &OS) {
-  std::vector<CommandOption> Options;
-  for (Record *R : Records)
-    Options.emplace_back(R);
+  std::vector<CommandOption> Options(Records.begin(), Records.end());
 
   std::string ID = Command;
   std::replace(ID.begin(), ID.end(), ' ', '_');
@@ -170,10 +168,11 @@ static void emitOptions(std::string Command, std::vector<Record *> Records,
   OS << "#endif // " << Command << " command\n\n";
 }
 
-void lldb_private::EmitOptionDefs(RecordKeeper &Records, raw_ostream &OS) {
+void lldb_private::EmitOptionDefs(const RecordKeeper &Records,
+                                  raw_ostream &OS) {
   emitSourceFileHeader("Options for LLDB command line commands.", OS, Records);
 
-  std::vector<Record *> Options = Records.getAllDerivedDefinitions("Option");
+  ArrayRef<const Record *> Options = Records.getAllDerivedDefinitions("Option");
   for (auto &CommandRecordPair : getRecordsByName(Options, "Command")) {
     emitOptions(CommandRecordPair.first, CommandRecordPair.second, OS);
   }
diff --git a/lldb/utils/TableGen/LLDBPropertyDefEmitter.cpp b/lldb/utils/TableGen/LLDBPropertyDefEmitter.cpp
index f27f0f39fbfd61..8df87fda9f7756 100644
--- a/lldb/utils/TableGen/LLDBPropertyDefEmitter.cpp
+++ b/lldb/utils/TableGen/LLDBPropertyDefEmitter.cpp
@@ -21,13 +21,13 @@
 using namespace llvm;
 using namespace lldb_private;
 
-static void emitPropertyEnum(Record *Property, raw_ostream &OS) {
+static void emitPropertyEnum(const Record *Property, raw_ostream &OS) {
   OS << "eProperty";
   OS << Property->getName();
   OS << ",\n";
 }
 
-static void emitProperty(Record *Property, raw_ostream &OS) {
+static void emitProperty(const Record *Property, raw_ostream &OS) {
   OS << "  {";
 
   // Emit the property name.
@@ -126,7 +126,7 @@ static void emitProperty(Record *Property, raw_ostream &OS) {
 
 /// Emits all property initializers to the raw_ostream.
 static void emityProperties(std::string PropertyName,
-                            std::vector<Record *> PropertyRecords,
+                            const std::vector<const Record *> &PropertyRecords,
                             raw_ostream &OS) {
   // Generate the macro that the user needs to define before including the
   // *.inc file.
@@ -139,7 +139,7 @@ static void emityProperties(std::string PropertyName,
   OS << "#ifdef " << NeededMacro << "\n";
   OS << "static constexpr PropertyDefinition g_" << PropertyName
      << "_properties[] = {\n";
-  for (Record *R : PropertyRecords)
+  for (const Record *R : PropertyRecords)
     emitProperty(R, OS);
   OS << "};\n";
   // We undefine the macro for the user like Clang's include files are doing it.
@@ -149,7 +149,7 @@ static void emityProperties(std::string PropertyName,
 
 /// Emits all property initializers to the raw_ostream.
 static void emitPropertyEnum(std::string PropertyName,
-                             std::vector<Record *> PropertyRecords,
+                             ArrayRef<const Record *> PropertyRecords,
                              raw_ostream &OS) {
   // Generate the macro that the user needs to define before including the
   // *.inc file.
@@ -160,28 +160,29 @@ static void emitPropertyEnum(std::string PropertyName,
   // user to define the macro for the options that are needed.
   OS << "// Property enum cases for " << PropertyName << "\n";
   OS << "#ifdef " << NeededMacro << "\n";
-  for (Record *R : PropertyRecords)
+  for (const Record *R : PropertyRecords)
     emitPropertyEnum(R, OS);
   // We undefine the macro for the user like Clang's include files are doing it.
   OS << "#undef " << NeededMacro << "\n";
   OS << "#endif // " << PropertyName << " Property\n\n";
 }
 
-void lldb_private::EmitPropertyDefs(RecordKeeper &Records, raw_ostream &OS) {
+void lldb_private::EmitPropertyDefs(const RecordKeeper &Records,
+                                    raw_ostream &OS) {
   emitSourceFileHeader("Property definitions for LLDB.", OS, Records);
 
-  std::vector<Record *> Properties =
+  ArrayRef<const Record *> Properties =
       Records.getAllDerivedDefinitions("Property");
   for (auto &PropertyRecordPair : getRecordsByName(Properties, "Definition")) {
     emityProperties(PropertyRecordPair.first, PropertyRecordPair.second, OS);
   }
 }
 
-void lldb_private::EmitPropertyEnumDefs(RecordKeeper &Records,
+void lldb_private::EmitPropertyEnumDefs(const RecordKeeper &Records,
                                         raw_ostream &OS) {
   emitSourceFileHeader("Property definition enum for LLDB.", OS, Records);
 
-  std::vector<Record *> Properties =
+  ArrayRef<const Record *> Properties =
       Records.getAllDerivedDefinitions("Property");
   for (auto &PropertyRecordPair : getRecordsByName(Properties, "Definition")) {
     emitPropertyEnum(PropertyRecordPair.first, PropertyRecordPair.second, OS);
diff --git a/lldb/utils/TableGen/LLDBTableGen.cpp b/lldb/utils/TableGen/LLDBTableGen.cpp
index bbd3f3d6c66c4d..0c06c93fbdc3d7 100644
--- a/lldb/utils/TableGen/LLDBTableGen.cpp
+++ b/lldb/utils/TableGen/LLDBTableGen.cpp
@@ -43,7 +43,7 @@ static cl::opt<ActionType> Action(
                clEnumValN(GenPropertyEnumDefs, "gen-lldb-property-enum-defs",
                           "Generate lldb property enum definitions")));
 
-static bool LLDBTableGenMain(raw_ostream &OS, RecordKeeper &Records) {
+static bool LLDBTableGenMain(raw_ostream &OS, const RecordKeeper &Records) {
   switch (Action) {
   case PrintRecords:
     OS << Records; // No argument, dump all contents
diff --git a/lldb/utils/TableGen/LLDBTableGenBackends.h b/lldb/utils/TableGen/LLDBTableGenBackends.h
index b60c4705de3ad9..a51b7bf7b60d03 100644
--- a/lldb/utils/TableGen/LLDBTableGenBackends.h
+++ b/lldb/utils/TableGen/LLDBTableGenBackends.h
@@ -29,9 +29,9 @@ using llvm::RecordKeeper;
 
 namespace lldb_private {
 
-void EmitOptionDefs(RecordKeeper &RK, raw_ostream &OS);
-void EmitPropertyDefs(RecordKeeper &RK, raw_ostream &OS);
-void EmitPropertyEnumDefs(RecordKeeper &RK, raw_ostream &OS);
+void EmitOptionDefs(const RecordKeeper &RK, raw_ostream &OS);
+void EmitPropertyDefs(const RecordKeeper &RK, raw_ostream &OS);
+void EmitPropertyEnumDefs(const RecordKeeper &RK, raw_ostream &OS);
 int EmitSBAPIDWARFEnum(int argc, char **argv);
 
 } // namespace lldb_private
diff --git a/lldb/utils/TableGen/LLDBTableGenUtils.cpp b/lldb/utils/TableGen/LLDBTableGenUtils.cpp
index 8b4c7581c98b4c..c6388e1ec2d265 100644
--- a/lldb/utils/TableGen/LLDBTableGenUtils.cpp
+++ b/lldb/utils/TableGen/LLDBTableGenUtils.cpp
@@ -12,10 +12,10 @@
 using namespace llvm;
 using namespace lldb_private;
 
-RecordsByName lldb_private::getRecordsByName(std::vector<Record *> Records,
+RecordsByName lldb_private::getRecordsByName(ArrayRef<const Record *> Records,
                                              StringRef Name) {
   RecordsByName Result;
-  for (Record *R : Records)
+  for (const Record *R : Records)
     Result[R->getValueAsString(Name).str()].push_back(R);
   return Result;
 }
diff --git a/lldb/utils/TableGen/LLDBTableGenUtils.h b/lldb/utils/TableGen/LLDBTableGenUtils.h
index 76f0df9a23f62a..a1bcd3c31142af 100644
--- a/lldb/utils/TableGen/LLDBTableGenUtils.h
+++ b/lldb/utils/TableGen/LLDBTableGenUtils.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_UTILS_TABLEGEN_LLDBTABLEGENUTILS_H
 #define LLDB_UTILS_TABLEGEN_LLDBTABLEGENUTILS_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include <map>
 #include <string>
@@ -23,10 +24,10 @@ namespace lldb_private {
 
 /// Map of names to their associated records. This map also ensures that our
 /// records are sorted in a deterministic way.
-typedef std::map<std::string, std::vector<llvm::Record *>> RecordsByName;
+typedef std::map<std::string, std::vector<const llvm::Record *>> RecordsByName;
 
 /// Return records grouped by name.
-RecordsByName getRecordsByName(std::vector<llvm::Record *> Records,
+RecordsByName getRecordsByName(llvm::ArrayRef<const llvm::Record *> Records,
                                llvm::StringRef);
 
 } // namespace lldb_private

DavidSpickett · 2024-09-06T09:16:05Z

Is this part of a larger effort, do you have a link to an RFC or previous PRs?

I assume the motivation is const correctness?

jurahul · 2024-09-06T13:30:44Z

Yes, the motivation is const correctness. Please see the discourse thread here: https://discourse.llvm.org/t/psa-planned-changes-to-tablegen-getallderiveddefinitions-api-potential-downstream-breakages/81089

DavidSpickett · 2024-09-09T07:58:59Z

Great, please add that link to the PR description and then this can land.

jurahul · 2024-09-09T11:00:45Z

Done.

[LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper

06fc167

Migrate LLDB TableGen backend to use const RecordKeeper.

jurahul marked this pull request as ready for review September 6, 2024 08:22

jurahul requested a review from JDevlieghere as a code owner September 6, 2024 08:22

jurahul requested a review from mshockwave September 6, 2024 08:22

llvmbot added the lldb label Sep 6, 2024

jurahul requested a review from DavidSpickett September 9, 2024 11:19

DavidSpickett approved these changes Sep 9, 2024

View reviewed changes

DavidSpickett merged commit 32cef07 into llvm:main Sep 9, 2024
11 checks passed

jurahul deleted the const_record_lldb branch September 9, 2024 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper #107536

[LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper #107536

Uh oh!

jurahul commented Sep 6, 2024 •

edited

Loading

Uh oh!

llvmbot commented Sep 6, 2024

Uh oh!

DavidSpickett commented Sep 6, 2024

Uh oh!

jurahul commented Sep 6, 2024

Uh oh!

DavidSpickett commented Sep 9, 2024

Uh oh!

jurahul commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

[LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper #107536

[LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper #107536

Uh oh!

Conversation

jurahul commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 6, 2024

Uh oh!

DavidSpickett commented Sep 6, 2024

Uh oh!

jurahul commented Sep 6, 2024

Uh oh!

DavidSpickett commented Sep 9, 2024

Uh oh!

jurahul commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

jurahul commented Sep 6, 2024 •

edited

Loading