Skip to content

Commit fced2f4

Browse files
ldrummldrumm
authored
[libclc] Prefer StringRef fixing string handling bugs (#8296)
In addition to slightly reducing the cognitive overhead of all the different string types, this fixes two real-live bugs: 1. `std::atoi` is impossible to use correctly. We'd incorrectly assume invalid address spaces mangling as AS0 because `atoi` return zero on error. This has been replaced with `StringRef::getAsInteger` 2. A crash handling nested names due to incorrect use of `string::find_first_not_of("__spv::")` which searches for the first character that is not one one of the list of characters given by the first argument. The intention was to split on that substring, which `StringRef` has easy interfaces for achieving. Names in nested namespaces can now be parsed without a crash
1 parent fbb885e commit fced2f4

File tree

1 file changed

+41
-35
lines changed

1 file changed

+41
-35
lines changed

‎libclc/utils/libclc-remangler/LibclcRemangler.cpp

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ static cl::opt<bool> TestRun("t", cl::desc("Enable test run"), cl::init(false),
100100
static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
101101

102102
namespace {
103+
inline StringRef asRef(StringView S) { return {S.begin(), S.size()}; }
103104
class BumpPointerAllocator {
104105
public:
105106
BumpPointerAllocator()
@@ -175,7 +176,7 @@ class DefaultAllocator {
175176
BumpPointerAllocator Alloc;
176177
};
177178

178-
clang::QualType getBaseType(StringView Name, clang::ASTContext *AST,
179+
clang::QualType getBaseType(StringRef Name, clang::ASTContext *AST,
179180
bool &IsVariadic) {
180181
clang::QualType Res;
181182
// First find the match against `QualType`...
@@ -249,8 +250,7 @@ clang::QualType getBaseType(StringView Name, clang::ASTContext *AST,
249250
else if (Name == "_BitInt")
250251
assert(false && "unhandled type name: _BitInt");
251252
else {
252-
StringRef const N{Name.begin(), Name.size()};
253-
auto &II = AST->Idents.get(N);
253+
auto &II = AST->Idents.get(Name);
254254
auto *DC = AST->getTranslationUnitDecl();
255255
auto *ED = EnumDecl::Create(*AST, DC, SourceLocation(), SourceLocation(),
256256
&II, nullptr, false, false, true);
@@ -284,10 +284,10 @@ class Remangler {
284284
assert(MangleContext->shouldMangleDeclName(FD) &&
285285
"It should always be possible to mangle libclc func.");
286286

287-
SmallString<256> Buffer;
288-
raw_svector_ostream Out(Buffer);
287+
std::string Buf;
288+
raw_string_ostream Out(Buf);
289289
MangleContext->mangleName(FD, Out);
290-
return std::string{Out.str()};
290+
return Buf;
291291
}
292292

293293
private:
@@ -339,15 +339,14 @@ class Remangler {
339339
(Encoding->getName()->getKind() == Node::Kind::KNameType ||
340340
Encoding->getName()->getKind() == Node::Kind::KNameWithTemplateArgs) &&
341341
"Expected KNameType or KNameWithTemplateArgs node.");
342-
std::string KernelNameStr;
342+
StringRef KernelName;
343343
if (Encoding->getName()->getKind() == Node::Kind::KNameType) {
344344
auto *NT = static_cast<const NameType *>(Encoding->getName());
345-
KernelNameStr.assign(NT->getBaseName().begin(), NT->getBaseName().size());
345+
KernelName = asRef(NT->getBaseName());
346346
} else {
347347
auto *NT = static_cast<const NameWithTemplateArgs *>(Encoding->getName());
348-
KernelNameStr.assign(NT->getBaseName().begin(), NT->getBaseName().size());
348+
KernelName = asRef(NT->getBaseName());
349349
}
350-
StringRef const KernelName(KernelNameStr);
351350
FD->setDeclName(&AST->Idents.get(KernelName));
352351

353352
// Construct the argument list.
@@ -481,9 +480,12 @@ class Remangler {
481480
assert(VecType->getDimension()->getKind() == Node::Kind::KNameType);
482481
const auto *Dims = static_cast<const itanium_demangle::NameType *>(
483482
VecType->getDimension());
484-
std::string const DN{Dims->getName().begin(), Dims->getName().size()};
485-
auto D = std::atoi(DN.c_str());
486-
PossibleKinds.push_back(NodeKindInfo(Kind, D));
483+
size_t DimNum;
484+
if (asRef(Dims->getName()).getAsInteger(10, DimNum) || !DimNum) {
485+
assert(false && "invalid vector size specifier");
486+
break;
487+
}
488+
PossibleKinds.push_back(NodeKindInfo(Kind, DimNum));
487489
return handleTypeNode(VecType->getBaseType(), PossibleKinds);
488490
}
489491
case Node::Kind::KBinaryFPType: {
@@ -493,27 +495,28 @@ class Remangler {
493495
const auto *NameTypeNode =
494496
static_cast<const itanium_demangle::NameType *>(
495497
BFPType->getDimension());
496-
std::string const D{NameTypeNode->getBaseName().begin(),
497-
NameTypeNode->getBaseName().size()};
498-
assert(D == "16" && "Unexpected binary floating point type.");
499-
(void)D;
498+
assert(asRef(NameTypeNode->getBaseName()) == "16" &&
499+
"Unexpected binary floating point type.");
500500
// BinaryFPType is encoded as: BinaryFPType(NameType("16")), manually
501501
// construct "_Float16" NamedType node so we can pass it directly to
502502
// handleLeafTypeNode.
503503
NameType const FP16{"_Float16"};
504-
return handleLeafTypeNode(&FP16, PossibleKinds);
504+
return handleLeafTypeNode(FP16.getName(), PossibleKinds);
505505
}
506506
case Node::Kind::KVendorExtQualType: {
507507
const auto *ExtQualType =
508508
static_cast<const itanium_demangle::VendorExtQualType *>(TypeNode);
509-
std::string const AS(ExtQualType->getExt().begin(),
510-
ExtQualType->getExt().size());
511-
if (AS.rfind("AS", 0) != 0) {
509+
StringRef AS = asRef(ExtQualType->getExt());
510+
if (!AS.starts_with("AS")) {
511+
assert(false && "Unexpected ExtQualType.");
512+
break;
513+
}
514+
size_t ASNum;
515+
if (AS.drop_front(2).getAsInteger(10, ASNum)) {
512516
assert(false && "Unexpected ExtQualType.");
513517
break;
514518
}
515-
auto ASVal = std::atoi(AS.c_str() + 2);
516-
PossibleKinds.push_back(NodeKindInfo(Kind, ASVal));
519+
PossibleKinds.push_back({Kind, ASNum});
517520
return handleTypeNode(ExtQualType->getTy(), PossibleKinds);
518521
}
519522
case Node::Kind::KQualType: {
@@ -523,17 +526,16 @@ class Remangler {
523526
}
524527
case Node::Kind::KNameType: {
525528
auto *NT = static_cast<const itanium_demangle::NameType *>(TypeNode);
526-
return handleLeafTypeNode(NT, PossibleKinds);
529+
return handleLeafTypeNode(NT->getName(), PossibleKinds);
527530
}
528531
case Node::Kind::KNestedName: {
529532
const auto *NN =
530533
static_cast<const itanium_demangle::NestedName *>(TypeNode);
531534
OutputBuffer QB;
532535
NN->Qual->print(QB);
533536
PossibleKinds.push_back({Kind, QB.getBuffer(), QB.getCurrentPosition()});
534-
const auto *TN =
535-
static_cast<const itanium_demangle::NameType *>(NN->Name);
536-
return handleLeafTypeNode(TN, PossibleKinds);
537+
auto *NT = static_cast<const itanium_demangle::NameType *>(NN->Name);
538+
return handleLeafTypeNode(NT->getName(), PossibleKinds);
537539
}
538540
default: {
539541
OutputBuffer ErrorTypeOut;
@@ -551,16 +553,20 @@ class Remangler {
551553
// Handle undecorated type that can be matched against `QualType`, also
552554
// returning if variadic.
553555
std::pair<clang::QualType, bool>
554-
handleLeafTypeNode(const NameType *NT,
556+
handleLeafTypeNode(StringView Name,
555557
SmallVector<NodeKindInfo> &PossibleKinds) {
556-
StringView Name = NT->getName();
558+
return handleLeafTypeNode(asRef(Name), PossibleKinds);
559+
}
560+
561+
std::pair<clang::QualType, bool>
562+
handleLeafTypeNode(StringRef Name, SmallVector<NodeKindInfo> &PossibleKinds) {
557563

558564
// When in test run, don't enable replacements and assert that re-mangled
559565
// name matches the original.
560566
if (!TestRun) {
561567
auto It = TypeReplacements.find(Name.begin());
562568
if (It != TypeReplacements.end())
563-
Name = StringView(It->second);
569+
Name = It->second;
564570
}
565571

566572
bool IsVariadic = false;
@@ -584,7 +590,7 @@ class Remangler {
584590
std::string const N{KNN->DataStr + " " +
585591
Res.getBaseTypeIdentifier()->getName().str()};
586592
if (NestedNamesQTMap.count(N) == 0) {
587-
assert(KNN->DataStr.rfind("__spv::", 0) == 0 &&
593+
assert(StringRef(KNN->DataStr).starts_with("__spv") &&
588594
"Unexpected nested prefix");
589595
SourceLocation const SL{};
590596
RecordDecl *RD = nullptr;
@@ -593,8 +599,8 @@ class Remangler {
593599
*AST, AST->getTranslationUnitDecl(), false, SL, SL,
594600
&AST->Idents.get("__spv", tok::TokenKind::identifier), nullptr,
595601
false);
596-
std::string StructName{KNN->DataStr};
597-
StructName.erase(0, StructName.find_first_not_of("__spv::"));
602+
std::string StructName =
603+
StringRef(KNN->DataStr).split("__spv::").second.str();
598604
auto *II = &AST->Idents.get(StructName, tok::TokenKind::identifier);
599605
RD = RecordDecl::Create(*AST, TTK_Struct, SpvNamespace, SL, SL, II);
600606
auto *NNS = NestedNameSpecifier::Create(*AST, nullptr, SpvNamespace);
@@ -783,7 +789,7 @@ class LibCLCRemangler : public ASTConsumer {
783789
}
784790

785791
private:
786-
bool createClones(llvm::Module *M, std::string OriginalMangledName,
792+
bool createClones(llvm::Module *M, StringRef OriginalMangledName,
787793
std::string RemangledName,
788794
const itanium_demangle::Node *FunctionTree,
789795
TargetTypeReplacements &Replacements) {
@@ -799,7 +805,7 @@ class LibCLCRemangler : public ASTConsumer {
799805
}
800806

801807
bool
802-
createCloneFromMap(llvm::Module *M, std::string OriginalName,
808+
createCloneFromMap(llvm::Module *M, StringRef OriginalName,
803809
const itanium_demangle::Node *FunctionTree,
804810
SmallDenseMap<const char *, const char *> TypeReplacements,
805811
bool CloneeTypeReplacement = false) {

0 commit comments

Comments
 (0)