caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[StrTable] Switch the option parser to llvm::StringTable (#123308)

Browse Source 
Now that we have a dedicated abstraction for string tables, switch the
option parser library's string table over to it rather than using a raw
`const char*`. Also try to use the `StringTable::Offset` type rather
than a raw `unsigned` where we can to avoid accidental increments or
other issues.

This is based on review feedback for the initial switch of options to a
string table. Happy to tweak or adjust if desired here.
This commit is contained in:
Chandler Carruth
2025-01-22 23:19:47 -08:00
committed by GitHub
parent 8eb99bbe6e
commit cd5694ecea
7 changed files with 88 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
clang/lib/Frontend/CompilerInvocation.cpp
View File

@@ -282,7 +282,7 @@ using ArgumentConsumer = CompilerInvocation::ArgumentConsumer;
#undef OPTTABLE_STR_TABLE_CODE
static llvm::StringRef lookupStrInTable(unsigned Offset) {
return &OptionStrTable[Offset];
return OptionStrTable[Offset];
}
#define SIMPLE_ENUM_VALUE_TABLE

3
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
View File

@@ -42,6 +42,7 @@
#include "lldb/Utility/Status.h"
#include "lldb/Utility/Timer.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringTable.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Threading.h"
@@ -1083,7 +1084,7 @@ void PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
if (!version.empty() && sdk_type != XcodeSDK::Type::Linux &&
sdk_type != XcodeSDK::Type::XROS) {
#define OPTION(PREFIX_OFFSET, NAME_OFFSET, VAR, ...) \
llvm::StringRef opt_##VAR = &OptionStrTable[NAME_OFFSET]; \
llvm::StringRef opt_##VAR = OptionStrTable[NAME_OFFSET]; \
(void)opt_##VAR;
#include "clang/Driver/Options.inc"
#undef OPTION

67
llvm/include/llvm/Option/OptTable.h
View File

@@ -12,6 +12,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringTable.h"
#include "llvm/Option/OptSpecifier.h"
#include "llvm/Support/StringSaver.h"
#include <cassert>
@@ -54,7 +55,7 @@ public:
/// Entry for a single option instance in the option data table.
struct Info {
unsigned PrefixesOffset;
unsigned PrefixedNameOffset;
StringTable::Offset PrefixedNameOffset;
const char *HelpText;
// Help text for specific visibilities. A list of pairs, where each pair
// is a list of visibilities and a specific help string for those
@@ -80,34 +81,37 @@ public:
bool hasNoPrefix() const { return PrefixesOffset == 0; }
unsigned getNumPrefixes(ArrayRef<unsigned> PrefixesTable) const {
return PrefixesTable[PrefixesOffset];
unsigned getNumPrefixes(ArrayRef<StringTable::Offset> PrefixesTable) const {
// We embed the number of prefixes in the value of the first offset.
return PrefixesTable[PrefixesOffset].value();
}
ArrayRef<unsigned>
getPrefixOffsets(ArrayRef<unsigned> PrefixesTable) const {
return hasNoPrefix() ? ArrayRef<unsigned>()
ArrayRef<StringTable::Offset>
getPrefixOffsets(ArrayRef<StringTable::Offset> PrefixesTable) const {
return hasNoPrefix() ? ArrayRef<StringTable::Offset>()
: PrefixesTable.slice(PrefixesOffset + 1,
getNumPrefixes(PrefixesTable));
}
void appendPrefixes(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
void appendPrefixes(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
SmallVectorImpl<StringRef> &Prefixes) const {
for (unsigned PrefixOffset : getPrefixOffsets(PrefixesTable))
Prefixes.push_back(&StrTable[PrefixOffset]);
for (auto PrefixOffset : getPrefixOffsets(PrefixesTable))
Prefixes.push_back(StrTable[PrefixOffset]);
}
StringRef getPrefix(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
StringRef getPrefix(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
unsigned PrefixIndex) const {
return &StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
return StrTable[getPrefixOffsets(PrefixesTable)[PrefixIndex]];
}
StringRef getPrefixedName(const char *StrTable) const {
return &StrTable[PrefixedNameOffset];
StringRef getPrefixedName(const StringTable &StrTable) const {
return StrTable[PrefixedNameOffset];
}
StringRef getName(const char *StrTable,
ArrayRef<unsigned> PrefixesTable) const {
StringRef getName(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable) const {
unsigned PrefixLength =
hasNoPrefix() ? 0 : getPrefix(StrTable, PrefixesTable, 0).size();
return getPrefixedName(StrTable).drop_front(PrefixLength);
@@ -117,13 +121,13 @@ public:
private:
// A unified string table for these options. Individual strings are stored as
// null terminated C-strings at offsets within this table.
const char *StrTable;
const StringTable *StrTable;
// A table of different sets of prefixes. Each set starts with the number of
// prefixes in that set followed by that many offsets into the string table
// for each of the prefix strings. This is essentially a Pascal-string style
// encoding.
ArrayRef<unsigned> PrefixesTable;
ArrayRef<StringTable::Offset> PrefixesTable;
/// The option information table.
ArrayRef<Info> OptionInfos;
@@ -161,7 +165,8 @@ private:
protected:
/// Initialize OptTable using Tablegen'ed OptionInfos. Child class must
/// manually call \c buildPrefixChars once they are fully constructed.
OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
OptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
/// Build (or rebuild) the PrefixChars member.
@@ -171,10 +176,12 @@ public:
virtual ~OptTable();
/// Return the string table used for option names.
const char *getStrTable() const { return StrTable; }
const StringTable &getStrTable() const { return *StrTable; }
/// Return the prefixes table used for option names.
ArrayRef<unsigned> getPrefixesTable() const { return PrefixesTable; }
ArrayRef<StringTable::Offset> getPrefixesTable() const {
return PrefixesTable;
}
/// Return the total number of option classes.
unsigned getNumOptions() const { return OptionInfos.size(); }
@@ -187,25 +194,25 @@ public:
/// Lookup the name of the given option.
StringRef getOptionName(OptSpecifier id) const {
return getInfo(id).getName(StrTable, PrefixesTable);
return getInfo(id).getName(*StrTable, PrefixesTable);
}
/// Lookup the prefix of the given option.
StringRef getOptionPrefix(OptSpecifier id) const {
const Info &I = getInfo(id);
return I.hasNoPrefix() ? StringRef()
: I.getPrefix(StrTable, PrefixesTable, 0);
: I.getPrefix(*StrTable, PrefixesTable, 0);
}
void appendOptionPrefixes(OptSpecifier id,
SmallVectorImpl<StringRef> &Prefixes) const {
const Info &I = getInfo(id);
I.appendPrefixes(StrTable, PrefixesTable, Prefixes);
I.appendPrefixes(*StrTable, PrefixesTable, Prefixes);
}
/// Lookup the prefixed name of the given option.
StringRef getOptionPrefixedName(OptSpecifier id) const {
return getInfo(id).getPrefixedName(StrTable);
return getInfo(id).getPrefixedName(*StrTable);
}
/// Get the kind of the given option.
@@ -418,19 +425,21 @@ private:
/// Specialization of OptTable
class GenericOptTable : public OptTable {
protected:
GenericOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
GenericOptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos, bool IgnoreCase = false);
};
class PrecomputedOptTable : public OptTable {
protected:
PrecomputedOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
PrecomputedOptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos,
ArrayRef<unsigned> PrefixesUnionOffsets,
ArrayRef<StringTable::Offset> PrefixesUnionOffsets,
bool IgnoreCase = false)
: OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
for (unsigned PrefixOffset : PrefixesUnionOffsets)
PrefixesUnion.push_back(&StrTable[PrefixOffset]);
for (auto PrefixOffset : PrefixesUnionOffsets)
PrefixesUnion.push_back(StrTable[PrefixOffset]);
buildPrefixChars();
}
};

70
llvm/lib/Option/OptTable.cpp
View File

@@ -33,11 +33,12 @@ using namespace llvm::opt;
namespace {
struct OptNameLess {
const char *StrTable;
ArrayRef<unsigned> PrefixesTable;
const StringTable *StrTable;
ArrayRef<StringTable::Offset> PrefixesTable;
explicit OptNameLess(const char *StrTable, ArrayRef<unsigned> PrefixesTable)
: StrTable(StrTable), PrefixesTable(PrefixesTable) {}
explicit OptNameLess(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable)
: StrTable(&StrTable), PrefixesTable(PrefixesTable) {}
#ifndef NDEBUG
inline bool operator()(const OptTable::Info &A,
@@ -45,13 +46,13 @@ struct OptNameLess {
if (&A == &B)
return false;
if (int Cmp = StrCmpOptionName(A.getName(StrTable, PrefixesTable),
B.getName(StrTable, PrefixesTable)))
if (int Cmp = StrCmpOptionName(A.getName(*StrTable, PrefixesTable),
B.getName(*StrTable, PrefixesTable)))
return Cmp < 0;
SmallVector<StringRef, 8> APrefixes, BPrefixes;
A.appendPrefixes(StrTable, PrefixesTable, APrefixes);
B.appendPrefixes(StrTable, PrefixesTable, BPrefixes);
A.appendPrefixes(*StrTable, PrefixesTable, APrefixes);
B.appendPrefixes(*StrTable, PrefixesTable, BPrefixes);
if (int Cmp = StrCmpOptionPrefixes(APrefixes, BPrefixes))
return Cmp < 0;
@@ -68,7 +69,7 @@ struct OptNameLess {
// Support lower_bound between info and an option name.
inline bool operator()(const OptTable::Info &I, StringRef Name) const {
// Do not fallback to case sensitive comparison.
return StrCmpOptionName(I.getName(StrTable, PrefixesTable), Name, false) <
return StrCmpOptionName(I.getName(*StrTable, PrefixesTable), Name, false) <
0;
}
};
@@ -76,9 +77,10 @@ struct OptNameLess {
OptSpecifier::OptSpecifier(const Option *Opt) : ID(Opt->getID()) {}
OptTable::OptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
OptTable::OptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos, bool IgnoreCase)
: StrTable(StrTable), PrefixesTable(PrefixesTable),
: StrTable(&StrTable), PrefixesTable(PrefixesTable),
OptionInfos(OptionInfos), IgnoreCase(IgnoreCase) {
// Explicitly zero initialize the error to work around a bug in array
// value-initialization on MinGW with gcc 4.3.5.
@@ -151,13 +153,13 @@ static bool isInput(const ArrayRef<StringRef> &Prefixes, StringRef Arg) {
}
/// \returns Matched size. 0 means no match.
static unsigned matchOption(const char *StrTable,
ArrayRef<unsigned> PrefixesTable,
static unsigned matchOption(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
const OptTable::Info *I, StringRef Str,
bool IgnoreCase) {
StringRef Name = I->getName(StrTable, PrefixesTable);
for (unsigned PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
StringRef Prefix = &StrTable[PrefixOffset];
for (auto PrefixOffset : I->getPrefixOffsets(PrefixesTable)) {
StringRef Prefix = StrTable[PrefixOffset];
if (Str.starts_with(Prefix)) {
StringRef Rest = Str.substr(Prefix.size());
bool Matched = IgnoreCase ? Rest.starts_with_insensitive(Name)
@@ -170,13 +172,13 @@ static unsigned matchOption(const char *StrTable,
}
// Returns true if one of the Prefixes + In.Names matches Option
static bool optionMatches(const char *StrTable,
ArrayRef<unsigned> PrefixesTable,
static bool optionMatches(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
const OptTable::Info &In, StringRef Option) {
StringRef Name = In.getName(StrTable, PrefixesTable);
if (Option.consume_back(Name))
for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable))
if (Option == &StrTable[PrefixOffset])
for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable))
if (Option == StrTable[PrefixOffset])
return true;
return false;
}
@@ -189,7 +191,7 @@ OptTable::suggestValueCompletions(StringRef Option, StringRef Arg) const {
// Search all options and return possible values.
for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
const Info &In = OptionInfos[I];
if (!In.Values || !optionMatches(StrTable, PrefixesTable, In, Option))
if (!In.Values || !optionMatches(*StrTable, PrefixesTable, In, Option))
continue;
SmallVector<StringRef, 8> Candidates;
@@ -217,9 +219,9 @@ OptTable::findByPrefix(StringRef Cur, Visibility VisibilityMask,
if (In.Flags & DisableFlags)
continue;
StringRef Name = In.getName(StrTable, PrefixesTable);
for (unsigned PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
StringRef Prefix = &StrTable[PrefixOffset];
StringRef Name = In.getName(*StrTable, PrefixesTable);
for (auto PrefixOffset : In.getPrefixOffsets(PrefixesTable)) {
StringRef Prefix = (*StrTable)[PrefixOffset];
std::string S = (Twine(Prefix) + Name + "\t").str();
if (In.HelpText)
S += In.HelpText;
@@ -271,7 +273,7 @@ unsigned OptTable::internalFindNearest(
for (const Info &CandidateInfo :
ArrayRef<Info>(OptionInfos).drop_front(FirstSearchableIndex)) {
StringRef CandidateName = CandidateInfo.getName(StrTable, PrefixesTable);
StringRef CandidateName = CandidateInfo.getName(*StrTable, PrefixesTable);
// We can eliminate some option prefix/name pairs as candidates right away:
// * Ignore option candidates with empty names, such as "--", or names
@@ -304,9 +306,9 @@ unsigned OptTable::internalFindNearest(
// Consider each possible prefix for each candidate to find the most
// appropriate one. For example, if a user asks for "--helm", suggest
// "--help" over "-help".
for (unsigned CandidatePrefixOffset :
for (auto CandidatePrefixOffset :
CandidateInfo.getPrefixOffsets(PrefixesTable)) {
StringRef CandidatePrefix = &StrTable[CandidatePrefixOffset];
StringRef CandidatePrefix = (*StrTable)[CandidatePrefixOffset];
// If Candidate and NormalizedName have more than 'BestDistance'
// characters of difference, no need to compute the edit distance, it's
// going to be greater than BestDistance. Don't bother computing Candidate
@@ -359,14 +361,14 @@ std::unique_ptr<Arg> OptTable::parseOneArgGrouped(InputArgList &Args,
StringRef Name = Str.ltrim(PrefixChars);
const Info *Start =
std::lower_bound(OptionInfos.data() + FirstSearchableIndex, End, Name,
OptNameLess(StrTable, PrefixesTable));
OptNameLess(*StrTable, PrefixesTable));
const Info *Fallback = nullptr;
unsigned Prev = Index;
// Search for the option which matches Str.
for (; Start != End; ++Start) {
unsigned ArgSize =
matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase);
matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase);
if (!ArgSize)
continue;
@@ -449,7 +451,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
// Search for the first next option which could be a prefix.
Start =
std::lower_bound(Start, End, Name, OptNameLess(StrTable, PrefixesTable));
std::lower_bound(Start, End, Name, OptNameLess(*StrTable, PrefixesTable));
// Options are stored in sorted order, with '\0' at the end of the
// alphabet. Since the only options which can accept a string must
@@ -464,7 +466,7 @@ std::unique_ptr<Arg> OptTable::internalParseOneArg(
// Scan for first option which is a proper prefix.
for (; Start != End; ++Start)
if ((ArgSize =
matchOption(StrTable, PrefixesTable, Start, Str, IgnoreCase)))
matchOption(*StrTable, PrefixesTable, Start, Str, IgnoreCase)))
break;
if (Start == End)
break;
@@ -787,15 +789,15 @@ void OptTable::internalPrintHelp(
OS.flush();
}
GenericOptTable::GenericOptTable(const char *StrTable,
ArrayRef<unsigned> PrefixesTable,
GenericOptTable::GenericOptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos, bool IgnoreCase)
: OptTable(StrTable, PrefixesTable, OptionInfos, IgnoreCase) {
std::set<StringRef> TmpPrefixesUnion;
for (auto const &Info : OptionInfos.drop_front(FirstSearchableIndex))
for (unsigned PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
TmpPrefixesUnion.insert(StringRef(&StrTable[PrefixOffset]));
for (auto PrefixOffset : Info.getPrefixOffsets(PrefixesTable))
TmpPrefixesUnion.insert(StrTable[PrefixOffset]);
PrefixesUnion.append(TmpPrefixesUnion.begin(), TmpPrefixesUnion.end());
buildPrefixChars();
}

3
llvm/tools/llvm-objdump/llvm-objdump.cpp
View File

@@ -94,7 +94,8 @@ namespace {
class CommonOptTable : public opt::GenericOptTable {
public:
CommonOptTable(const char *StrTable, ArrayRef<unsigned> PrefixesTable,
CommonOptTable(const StringTable &StrTable,
ArrayRef<StringTable::Offset> PrefixesTable,
ArrayRef<Info> OptionInfos, const char *Usage,
const char *Description)
: opt::GenericOptTable(StrTable, PrefixesTable, OptionInfos),

3
llvm/unittests/Option/OptionMarshallingTest.cpp
View File

@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringTable.h"
#include "gtest/gtest.h"
#define OPTTABLE_STR_TABLE_CODE
@@ -20,7 +21,7 @@ struct OptionWithMarshallingInfo {
const char *ImpliedValue;
llvm::StringRef getPrefixedName() const {
return &OptionStrTable[PrefixedNameOffset];
return OptionStrTable[PrefixedNameOffset];
}
};

11
llvm/utils/TableGen/OptionParserEmitter.cpp
View File

@@ -303,15 +303,17 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
OS << "/////////\n";
OS << "// String table\n\n";
OS << "#ifdef OPTTABLE_STR_TABLE_CODE\n";
Table.EmitStringLiteralDef(OS, "static constexpr char OptionStrTable[]",
/*Indent=*/"");
Table.EmitStringLiteralDef(
OS, "static constexpr llvm::StringTable OptionStrTable",
/*Indent=*/"");
OS << "#endif // OPTTABLE_STR_TABLE_CODE\n\n";
// Dump prefixes.
OS << "/////////\n";
OS << "// Prefixes\n\n";
OS << "#ifdef OPTTABLE_PREFIXES_TABLE_CODE\n";
OS << "static constexpr unsigned OptionPrefixesTable[] = {\n";
OS << "static constexpr llvm::StringTable::Offset OptionPrefixesTable[] = "
"{\n";
{
// Ensure the first prefix set is always empty.
assert(!Prefixes.empty() &&
@@ -339,7 +341,8 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
OS << "/////////\n";
OS << "// Prefix Union\n\n";
OS << "#ifdef OPTTABLE_PREFIXES_UNION_CODE\n";
OS << "static constexpr unsigned OptionPrefixesUnion[] = {\n";
OS << "static constexpr llvm::StringTable::Offset OptionPrefixesUnion[] = "
"{\n";
{
llvm::ListSeparator Sep(", ");
for (auto Prefix : PrefixesUnion)