llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-llvm-binary-utilities Author: Chandler Carruth (chandlerc) <details> <summary>Changes</summary> 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. --- Patch is 20.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123308.diff 7 Files Affected: - (modified) clang/lib/Frontend/CompilerInvocation.cpp (+1-1) - (modified) lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp (+2-1) - (modified) llvm/include/llvm/Option/OptTable.h (+38-29) - (modified) llvm/lib/Option/OptTable.cpp (+36-34) - (modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+2-1) - (modified) llvm/unittests/Option/OptionMarshallingTest.cpp (+2-1) - (modified) llvm/utils/TableGen/OptionParserEmitter.cpp (+7-4) ``````````diff diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index 58658dedbaf1ee..3bf124e4827be9 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -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 diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp index 2a36f95c94d0ce..51e9a6d81b8390 100644 --- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp +++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp @@ -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 diff --git a/llvm/include/llvm/Option/OptTable.h b/llvm/include/llvm/Option/OptTable.h index 38a03fef7ae124..61a58aa304ecb4 100644 --- a/llvm/include/llvm/Option/OptTable.h +++ b/llvm/include/llvm/Option/OptTable.h @@ -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 @@ class OptTable { /// 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 @@ class OptTable { 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 @@ class OptTable { 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 @@ class OptTable { 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 @@ class OptTable { 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 @@ class OptTable { /// 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 @@ class OptTable { /// 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(); } }; diff --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp index 87e6f1f12364c2..6d10e6154147ec 100644 --- a/llvm/lib/Option/OptTable.cpp +++ b/llvm/lib/Option/OptTable.cpp @@ -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(); } diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp index 93fed8ee8e6f42..99e0440dce78d5 100644 --- a/llvm/tools/llvm-objdump/llvm-objdump.cpp +++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp @@ -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), diff --git a/llvm/unittests/Option/OptionMarshallingTest.cpp b/llvm/unittests/Option/OptionMarshallingTest.cpp index 08c3b019689f8c..005144b91bf7f3 100644 --- a/llvm/unittests/Option/OptionMarshallingTest.cpp +++ b/llvm/unittests/Option/OptionMarshallingTest.cpp @@ -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]; } }; diff --git a/llvm/utils/TableGen/OptionParserEmitter.cpp b/llvm/utils/TableGen/OptionParserEmitter.cpp index 8b92d252392194..35a452890b0ec7 100644 --- a/llvm/utils/TableGen/OptionParserEmitter.cpp +++ b/llvm/utils/TableGen/OptionParserEmitter.cpp @@ -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"; // ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/123308 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
