Add a super simple wrapper for a merged string table. #119488

chandlerc · 2024-12-11T02:53:17Z

Suggestions welcome on what to better name this -- StringTable as I currently have it seems too general, but wasn't sure what other name would be better.

It currently has a very minimal API. I'm happy to expand it if folks have ideas for what API would be useful, but this actually seemed like it might be all we really need.

llvmbot · 2024-12-11T02:53:51Z

@llvm/pr-subscribers-llvm-adt

Author: Chandler Carruth (chandlerc)

Changes

Suggestions welcome on what to better name this -- StringTable as I currently have it seems too general, but wasn't sure what other name would be better.

It currently has a very minimal API. I'm happy to expand it if folks have ideas for what API would be useful, but this actually seemed like it might be all we really need.

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

3 Files Affected:

(added) llvm/include/llvm/ADT/StringTable.h (+83)
(modified) llvm/unittests/ADT/CMakeLists.txt (+1)
(added) llvm/unittests/ADT/StringTableTest.cpp (+35)

diff --git a/llvm/include/llvm/ADT/StringTable.h b/llvm/include/llvm/ADT/StringTable.h
new file mode 100644
index 00000000000000..29f29624ea344a
--- /dev/null
+++ b/llvm/include/llvm/ADT/StringTable.h
@@ -0,0 +1,83 @@
+//===- StringTable.h - Table of strings tracked by offset ----------C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ADT_STRING_TABLE_H
+#define LLVM_ADT_STRING_TABLE_H
+
+#include "llvm/ADT/StringRef.h"
+
+namespace llvm {
+
+/// A table of densely packed, null-terminated strings indexed by offset.
+///
+/// This table abstracts a densely concatenated list of null-terminated strings,
+/// each of which can be referenced using an offset into the table.
+///
+/// This requires and ensures that the string at offset 0 is also the empty
+/// string. This helps allow zero-initialized offsets form empty strings and
+/// avoids non-zero initialization when using a string literal pointer would
+/// allow a null pointer.
+///
+/// The primary use case is having a single global string literal for the table
+/// contents, and offsets into it in other global data structures to avoid
+/// dynamic relocations of individual string literal pointers in those global
+/// data structures.
+class StringTable {
+  StringRef Table;
+
+public:
+  // An offset into one of these packed string tables, used to select a string
+  // within the table.
+  //
+  // Typically these are created by TableGen or other code generator from
+  // computed offsets, and it just wraps that integer into a type until it is
+  // used with the relevant table.
+  //
+  // We also ensure that the empty string is at offset zero and default
+  // constructing this class gives you an offset of zero. This makes default
+  // constructing this type work similarly (after indexing the table) to default
+  // constructing a `StringRef`.
+  class Offset {
+    // Note that we ensure the empty string is at offset zero.
+    unsigned Value = 0;
+
+  public:
+    Offset() = default;
+    Offset(unsigned Value) : Value(Value) {}
+
+    unsigned value() const { return Value; }
+  };
+
+  // We directly handle string literals with a templated converting constructor
+  // because we *don't* want to do `strlen` on them -- we fully expect null
+  // bytes in this input. This is somewhat the opposite of how `StringLiteral`
+  // works.
+  template <size_t N>
+  constexpr StringTable(const char (&RawTable)[N]) : Table(RawTable, N) {
+    assert(!Table.empty() && "Requires at least a valid empty string.");
+    assert(Table[0] == '\0' && "Offset zero must be the empty string.");
+    // Ensure that `strlen` from any offset cannot overflow the end of the table
+    // by insisting on a null byte at the end.
+    assert(Table.back() == '\0' && "Last byte must be a null byte.");
+  }
+
+  // Get a string from the table starting with the provided offset. The returned
+  // `StringRef` is in fact null terminated, and so can be converted safely to a
+  // C-string if necessary for a system API.
+  StringRef operator[](Offset O) const {
+    assert(O.value() < Table.size() && "Out of bounds offset!");
+    return Table.data() + O.value();
+  }
+
+  /// Returns the byte size of the table.
+  size_t size() const { return Table.size(); }
+};
+
+} // namespace llvm
+
+#endif // LLVM_ADT_STRING_TABLE_H
diff --git a/llvm/unittests/ADT/CMakeLists.txt b/llvm/unittests/ADT/CMakeLists.txt
index c9bc58f45f08cf..07568ad0c64e33 100644
--- a/llvm/unittests/ADT/CMakeLists.txt
+++ b/llvm/unittests/ADT/CMakeLists.txt
@@ -86,6 +86,7 @@ add_llvm_unittest(ADTTests
   StringRefTest.cpp
   StringSetTest.cpp
   StringSwitchTest.cpp
+  StringTableTest.cpp
   TinyPtrVectorTest.cpp
   TrieRawHashMapTest.cpp
   TwineTest.cpp
diff --git a/llvm/unittests/ADT/StringTableTest.cpp b/llvm/unittests/ADT/StringTableTest.cpp
new file mode 100644
index 00000000000000..bf2cab2670d306
--- /dev/null
+++ b/llvm/unittests/ADT/StringTableTest.cpp
@@ -0,0 +1,35 @@
+//===- llvm/unittest/ADT/StringTableTest.cpp - StringTable tests ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/StringTable.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <cstdlib>
+
+using namespace llvm;
+
+namespace {
+
+using ::testing::Eq;
+using ::testing::StrEq;
+
+TEST(StringTableTest, Basic) {
+  constexpr char InputTable[] = "\0test\0";
+  StringTable T = InputTable;
+
+  EXPECT_THAT(T.size(), Eq(sizeof(InputTable)));
+
+  EXPECT_THAT(T[0], Eq(""));
+  EXPECT_THAT(T[StringTable::Offset()], Eq(""));
+  EXPECT_THAT(T[1], Eq("test"));
+
+  // Also check that this is a valid C-string.
+  EXPECT_THAT(T[1].data(), StrEq("test"));
+}
+
+} // anonymous namespace

chandlerc · 2024-12-11T02:55:17Z

Note that this is intended for use cases like #119198 and some of the other string tables indexed by offset.

rnk

Thanks, looks good with suggestions

rnk · 2024-12-11T17:53:52Z

llvm/include/llvm/ADT/StringTable.h

+
+  public:
+    Offset() = default;
+    Offset(unsigned Value) : Value(Value) {}


This should be constexpr, so it can won't require dynamic initialization, right?

Done and test enhanced to verify this.

rnk · 2024-12-11T17:56:29Z

llvm/include/llvm/ADT/StringTable.h

+  // works.
+  template <size_t N>
+  constexpr StringTable(const char (&RawTable)[N]) : Table(RawTable, N) {
+    assert(!Table.empty() && "Requires at least a valid empty string.");


Are assertions safe in a constexpr-context?

It would also be reasonable to static_assert(N < UINT_MAX, "max string table size is 4GiB"); to rationalize the choice of 32-bit offsets

Yep, as long as they don't fire.

That said, these weren't because StringRef for some reason only supports constexpr calls to empty, size, and data. I've rewritten this to work in constexpr and tested it.

Also added the static assert, seems nice.

rnk · 2024-12-11T17:57:35Z

llvm/include/llvm/ADT/StringTable.h

+  // constructing this class gives you an offset of zero. This makes default
+  // constructing this type work similarly (after indexing the table) to default
+  // constructing a `StringRef`.
+  class Offset {


Bikeshedding nit: I tend to prefer non-nested classes but I don't feel strongly about it.

I'm fine either way TBH... But I didn't have a better name than StringTableOffset, and at that point it seemed like it should be nested.

If you have a name suggestion, happy to move it out?

I would go for StrTabOffset, but I'm more comfortable with consonant cluster mush than most people.

Yeah, I think I'll leave this as StringTable::Offset for now. =D

Thanks again, will merge once other things are fixed, and start working on PRs to switch over.

rnk · 2024-12-11T18:50:02Z

Re: The name StringTable: I couldn't come up with a better one. To me it does capture the idea of a sequence of null-terminated strings well, though.

Suggestions welcome on what to better name this -- `StringTable` as I currently have it seems too general, but wasn't sure what other name would be better. It currently has a *very* minimal API. I'm happy to expand it if folks have ideas for what API would be useful, but this actually seemed like it might be all we really need.

carlocab · 2024-12-12T09:26:00Z

llvm/include/llvm/ADT/StringTable.h

+  constexpr StringRef operator[](Offset O) const {
+    assert(O.value() < Table.size() && "Out of bounds offset!");
+    return Table.data() + O.value();
+  }


Wonder if it's worth asserting that the byte just before O is the null byte. Not sure if there's a use-case for indexing into a StringTable in the middle of one of the strings, though.

jayfoad · 2024-12-17T11:14:50Z

Note that we already have include/llvm/TableGen/StringToOffsetTable.h and utils/TableGen/Basic/SequenceToOffsetTable.h. It would be good to consolidate all of these somehow, bearing in mind that there are different use cases: sequence types other than "string", sequences that don't need a terminator, sequences that should share common suffixes where possible, etc.

chandlerc requested review from rnk and dwblaikie December 11, 2024 02:53

llvmbot added the llvm:adt label Dec 11, 2024

chandlerc mentioned this pull request Dec 11, 2024

Rework the Option library to reduce dynamic relocations #119198

Merged

rnk approved these changes Dec 11, 2024

View reviewed changes

chandlerc force-pushed the strtable branch from 4e7deb6 to fb7bc41 Compare December 12, 2024 01:37

chandlerc merged commit ef28e96 into llvm:main Dec 12, 2024
8 checks passed

carlocab reviewed Dec 12, 2024

View reviewed changes

kuhar mentioned this pull request Jan 9, 2025

Remove StringLiteral in favor of StringRef #122366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add a super simple wrapper for a merged string table. #119488

Add a super simple wrapper for a merged string table. #119488

Uh oh!

chandlerc commented Dec 11, 2024

Uh oh!

llvmbot commented Dec 11, 2024

Uh oh!

chandlerc commented Dec 11, 2024

Uh oh!

rnk left a comment

Uh oh!

rnk Dec 11, 2024

Uh oh!

chandlerc Dec 12, 2024

Uh oh!

rnk Dec 11, 2024

Uh oh!

chandlerc Dec 12, 2024

Uh oh!

rnk Dec 11, 2024

Uh oh!

chandlerc Dec 11, 2024

Uh oh!

rnk Dec 11, 2024

Uh oh!

chandlerc Dec 12, 2024

Uh oh!

rnk commented Dec 11, 2024

Uh oh!

Uh oh!

carlocab Dec 12, 2024

Uh oh!

jayfoad commented Dec 17, 2024

Uh oh!

Uh oh!

Add a super simple wrapper for a merged string table. #119488

Add a super simple wrapper for a merged string table. #119488

Uh oh!

Conversation

chandlerc commented Dec 11, 2024

Uh oh!

llvmbot commented Dec 11, 2024

Uh oh!

chandlerc commented Dec 11, 2024

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnk commented Dec 11, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayfoad commented Dec 17, 2024

Uh oh!

Uh oh!