https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/227654
>From ffcd3f19df5bc8e989c31bd7aa840dcb38edea6d Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Wed, 30 Sep 2026 10:41:35 +0000 Subject: [PATCH 1/3] [Remarks] Escape control characters in YAML remark arguments and decode them when parsing The serializer writes argument values with more than one newline as literal block scalars. A block scalar has no escapes, so a value that also contained a control character other than tab or newline was written with the raw byte, which strict YAML readers reject. Such values now use the double-quoted form, as do values whose first line starts with a space, which a block scalar would read as indentation. The parser took the raw scalar text and stripped only single quotes, so double-quoted values came back with their quotes and escapes, and '' inside single quotes was not unescaped. Decode scalars with ScalarNode::getValue instead and report escape errors. Unescaped values and block scalar values are copied into storage owned by the parser. Block scalar values used to point into the YAML document, which next() frees before returning the remark. Assisted-by: Claude --- llvm/lib/Remarks/YAMLRemarkParser.cpp | 31 +++++----- llvm/lib/Remarks/YAMLRemarkParser.h | 4 ++ llvm/lib/Remarks/YAMLRemarkSerializer.cpp | 16 ++++- .../Remarks/YAMLRemarksParsingTest.cpp | 51 ++++++++++++++++ .../Remarks/YAMLRemarksSerializerTest.cpp | 61 +++++++++++++++++++ 5 files changed, 146 insertions(+), 17 deletions(-) diff --git a/llvm/lib/Remarks/YAMLRemarkParser.cpp b/llvm/lib/Remarks/YAMLRemarkParser.cpp index 33f659eaaa49b..b1ba56c042a6f 100644 --- a/llvm/lib/Remarks/YAMLRemarkParser.cpp +++ b/llvm/lib/Remarks/YAMLRemarkParser.cpp @@ -265,22 +265,21 @@ Expected<StringRef> YAMLRemarkParser::parseKey(yaml::KeyValueNode &Node) { } Expected<StringRef> YAMLRemarkParser::parseStr(yaml::KeyValueNode &Node) { - auto *Value = dyn_cast_if_present<yaml::ScalarNode>(Node.getValue()); - yaml::BlockScalarNode *ValueBlock; - StringRef Result; - if (!Value) { - // Try to parse the value as a block node. - ValueBlock = dyn_cast_if_present<yaml::BlockScalarNode>(Node.getValue()); - if (!ValueBlock) - return error("expected a value of scalar type.", Node); - Result = ValueBlock->getValue(); - } else - Result = Value->getRawValue(); - - Result.consume_front("\'"); - Result.consume_back("\'"); - - return Result; + yaml::Node *Value = Node.getValue(); + // A block value lives in the YAML document, which next() frees before + // returning the remark. + if (auto *Block = dyn_cast_if_present<yaml::BlockScalarNode>(Value)) + return Block->getValue().copy(Alloc); + + auto *Scalar = dyn_cast_if_present<yaml::ScalarNode>(Value); + if (!Scalar) + return error("expected a value of scalar type.", Node); + SmallString<32> Storage; + StringRef Result = Scalar->getValue(Storage); + if (Error E = error()) + return std::move(E); + // getValue only fills Storage when it had to rewrite the value. + return Storage.empty() ? Result : Result.copy(Alloc); } Expected<unsigned> YAMLRemarkParser::parseUnsigned(yaml::KeyValueNode &Node) { diff --git a/llvm/lib/Remarks/YAMLRemarkParser.h b/llvm/lib/Remarks/YAMLRemarkParser.h index 9a30e9e295cb2..7de905b2a5bf8 100644 --- a/llvm/lib/Remarks/YAMLRemarkParser.h +++ b/llvm/lib/Remarks/YAMLRemarkParser.h @@ -15,6 +15,7 @@ #include "llvm/Remarks/Remark.h" #include "llvm/Remarks/RemarkParser.h" +#include "llvm/Support/Allocator.h" #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/SourceMgr.h" @@ -58,6 +59,9 @@ struct YAMLRemarkParser : public RemarkParser { /// If we parse remark metadata in separate mode, we need to open a new file /// and parse that. std::unique_ptr<MemoryBuffer> SeparateBuf; + /// Storage for values that do not point into the input buffer: unescaped + /// scalars and block scalars. Remarks point into it. + BumpPtrAllocator Alloc; YAMLRemarkParser(StringRef Buf); diff --git a/llvm/lib/Remarks/YAMLRemarkSerializer.cpp b/llvm/lib/Remarks/YAMLRemarkSerializer.cpp index 22e297040575c..39107f665e73f 100644 --- a/llvm/lib/Remarks/YAMLRemarkSerializer.cpp +++ b/llvm/lib/Remarks/YAMLRemarkSerializer.cpp @@ -12,6 +12,8 @@ //===----------------------------------------------------------------------===// #include "llvm/Remarks/YAMLRemarkSerializer.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/Remarks/Remark.h" #include "llvm/Support/FileSystem.h" #include <optional> @@ -109,6 +111,18 @@ template <typename T> struct SequenceTraits<ArrayRef<T>> { } }; +/// A literal block scalar has no escapes, so it cannot hold control characters +/// other than tab and line feed. UTF-8 is written as-is. Readers take the +/// indentation from the first non-empty line, so that line cannot start with a +/// space. +static bool canUseBlockScalar(StringRef S) { + if (S.ltrim('\n').starts_with(' ')) + return false; + return all_of(S, [](unsigned char C) { + return isPrint(C) || C == '\t' || C == '\n' || C >= 0x80; + }); +} + /// Implement this as a mapping for now to get proper quotation for the value. template <> struct MappingTraits<Argument> { static void mapping(IO &io, Argument &A) { @@ -116,7 +130,7 @@ template <> struct MappingTraits<Argument> { // NB: A.Key.data() is not necessarily null-terminated, as the StringRef may // be a span into the middle of a string. - if (StringRef(A.Val).count('\n') > 1) { + if (StringRef(A.Val).count('\n') > 1 && canUseBlockScalar(A.Val)) { StringBlockVal S(A.Val); io.mapRequired(A.Key, S); } else { diff --git a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp index 824813aa5af7c..29d378058717c 100644 --- a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp @@ -384,6 +384,16 @@ TEST(YAMLRemarks, ParsingWrongArgs) { " - DebugLoc: { File: a, Line: 1, Column: 2 }\n" "", "argument key is missing.")); + // Bad escape in a double-quoted value. + EXPECT_TRUE(parseExpectError("\n" + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "Args:\n" + " - Str: \"a\\qb\"\n" + "", + "Unrecognized escape code")); } static inline StringRef checkStr(StringRef Str, unsigned ExpectedLen) { @@ -475,6 +485,47 @@ TEST(YAMLRemarks, Contents) { EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors. } +TEST(YAMLRemarks, ContentsQuoted) { + StringRef Buf = "--- !Missed\n" + "Pass: pass\n" + "Name: name\n" + "Function: func\n" + "Args:\n" + " - Single: 'it''s'\n" + " - Double: \"abc\\ndef\\n\\x01ghi\"\n" + " - Block: |\n" + " 'abc'\n" + " def\n" + "--- !Missed\n" + "Pass: pass\n" + "Name: name\n" + "Function: func\n" + "Args:\n" + " - Block: |\n" + " xxxxxxxxxx\n" + " xxxxxxxxxx\n" + "\n"; + + Expected<std::unique_ptr<remarks::RemarkParser>> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf); + EXPECT_FALSE(errorToBool(MaybeParser.takeError())); + EXPECT_TRUE(*MaybeParser != nullptr); + + remarks::RemarkParser &Parser = **MaybeParser; + Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = Parser.next(); + EXPECT_FALSE(errorToBool(MaybeRemark.takeError())); + EXPECT_TRUE(*MaybeRemark != nullptr); + // The values must outlive the YAML document they were parsed from. + Expected<std::unique_ptr<remarks::Remark>> MaybeNext = Parser.next(); + EXPECT_FALSE(errorToBool(MaybeNext.takeError())); + + const remarks::Remark &Remark = **MaybeRemark; + ASSERT_EQ(Remark.Args.size(), 3U); + EXPECT_EQ(checkStr(Remark.Args[0].Val, 4), "it's"); + EXPECT_EQ(checkStr(Remark.Args[1].Val, 12), "abc\ndef\n\x01ghi"); + EXPECT_EQ(checkStr(Remark.Args[2].Val, 10), "'abc'\ndef\n"); +} + static inline StringRef checkStr(LLVMRemarkStringRef Str, unsigned ExpectedLen) { const char *StrData = LLVMRemarkStringGetData(Str); diff --git a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp index 974356d9cf30a..7da6e8ed4f1c8 100644 --- a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp @@ -193,3 +193,64 @@ TEST(YAMLRemarks, SerializerRemarkStringRefOOBRead) { " DebugLoc: { File: argpath, Line: 6, Column: 7 }\n" "...\n"); } + +TEST(YAMLRemarks, SerializerRemarkMultiLineArg) { + remarks::Remark R; + R.RemarkType = remarks::Type::Missed; + R.PassName = "pass"; + R.RemarkName = "name"; + R.FunctionName = "func"; + R.Args.emplace_back(); + R.Args.back().Key = "block"; + R.Args.back().Val = "abc\ndef\nghi"; + // A literal block scalar cannot hold control characters, so this has to be + // escaped in a double-quoted scalar instead. + R.Args.emplace_back(); + R.Args.back().Key = "control"; + R.Args.back().Val = "abc\ndef\n\x01ghi"; + checkStandalone(remarks::Format::YAML, R, + "--- !Missed\n" + "Pass: pass\n" + "Name: name\n" + "Function: func\n" + "Args:\n" + " - block: |\n" + " abc\n" + " def\n" + " ghi\n" + " - control: \"abc\\ndef\\n\\x01ghi\"\n" + "...\n"); +} + +TEST(YAMLRemarks, SerializerRemarkRoundTrip) { + // Values that the literal block form cannot hold must survive a round trip. + StringRef Vals[] = {"abc\ndef\n\x01ghi", "abc\r\ndef\r\nghi", + StringRef("abc\ndef\n\0ghi", 12), " abc\ndef\nghi", + "abc\ndef\nghi\n"}; + remarks::Remark R; + R.RemarkType = remarks::Type::Missed; + R.PassName = "pass"; + R.RemarkName = "name"; + R.FunctionName = "func"; + for (StringRef Val : Vals) { + R.Args.emplace_back(); + R.Args.back().Key = "key"; + R.Args.back().Val = Val; + } + + std::string Buf; + raw_string_ostream OS(Buf); + Expected<std::unique_ptr<remarks::RemarkSerializer>> MaybeS = + createRemarkSerializer(remarks::Format::YAML, OS); + ASSERT_FALSE(errorToBool(MaybeS.takeError())); + (*MaybeS)->emit(R); + (*MaybeS)->finalize(); + + Expected<std::unique_ptr<remarks::RemarkParser>> MaybeParser = + remarks::createRemarkParser(remarks::Format::YAML, Buf); + ASSERT_FALSE(errorToBool(MaybeParser.takeError())); + Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = + (*MaybeParser)->next(); + ASSERT_FALSE(errorToBool(MaybeRemark.takeError())); + EXPECT_EQ(**MaybeRemark, R); +} >From 459416d5065b10d42abef0e308545c7d7cf61763 Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Thu, 1 Oct 2026 13:20:12 +0000 Subject: [PATCH 2/3] [Remarks] Reject null characters in YAML remark strings The bitstream string table stores NUL-terminated strings, so a string with an embedded NUL splits into several entries and every later string is read back at the wrong index. Now that the YAML parser decodes escapes, "\0" in YAML input reaches that path. Reject it with a parse error. Assisted-by: Claude --- llvm/lib/Remarks/YAMLRemarkParser.cpp | 3 +++ .../tools/llvm-remarkutil/yaml2bitstream-null.test | 12 ++++++++++++ llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp | 11 +++++++++++ llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp | 3 +-- 4 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test diff --git a/llvm/lib/Remarks/YAMLRemarkParser.cpp b/llvm/lib/Remarks/YAMLRemarkParser.cpp index b1ba56c042a6f..31d255b56d55b 100644 --- a/llvm/lib/Remarks/YAMLRemarkParser.cpp +++ b/llvm/lib/Remarks/YAMLRemarkParser.cpp @@ -278,6 +278,9 @@ Expected<StringRef> YAMLRemarkParser::parseStr(yaml::KeyValueNode &Node) { StringRef Result = Scalar->getValue(Storage); if (Error E = error()) return std::move(E); + // The bitstream format cannot hold a null character in a string. + if (Result.contains('\0')) + return error("remark strings cannot contain null characters.", *Scalar); // getValue only fills Storage when it had to rewrite the value. return Storage.empty() ? Result : Result.copy(Alloc); } diff --git a/llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test b/llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test new file mode 100644 index 0000000000000..6cd09e374f412 --- /dev/null +++ b/llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test @@ -0,0 +1,12 @@ +# The bitstream string table cannot hold a null character, so reject one in +# YAML input instead of writing a corrupt string table. +# RUN: not llvm-remarkutil yaml2bitstream %s -o /dev/null 2>&1 | FileCheck %s +# CHECK: error: {{.*}}remark strings cannot contain null characters. + +--- !Missed +Pass: pass +Name: name +Function: func +Args: + - String: "abc\0def" +... diff --git a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp index c3bbdb7f971a3..f74c9958e0ff3 100644 --- a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp @@ -394,6 +394,17 @@ TEST(YAMLRemarks, ParsingWrongArgs) { " - Str: \"a\\qb\"\n" "", "Unrecognized escape code")); + // Null character in a value. + EXPECT_TRUE( + parseExpectError("\n" + "--- !Missed\n" + "Pass: inline\n" + "Name: NoDefinition\n" + "Function: foo\n" + "Args:\n" + " - Str: \"a\\0b\"\n" + "", + "remark strings cannot contain null characters.")); } static inline StringRef checkStr(StringRef Str, unsigned ExpectedLen) { diff --git a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp index 7da6e8ed4f1c8..53ca5d2fe2562 100644 --- a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp @@ -225,8 +225,7 @@ TEST(YAMLRemarks, SerializerRemarkMultiLineArg) { TEST(YAMLRemarks, SerializerRemarkRoundTrip) { // Values that the literal block form cannot hold must survive a round trip. StringRef Vals[] = {"abc\ndef\n\x01ghi", "abc\r\ndef\r\nghi", - StringRef("abc\ndef\n\0ghi", 12), " abc\ndef\nghi", - "abc\ndef\nghi\n"}; + " abc\ndef\nghi", "abc\ndef\nghi\n"}; remarks::Remark R; R.RemarkType = remarks::Type::Missed; R.PassName = "pass"; >From c3d85f713cd665f68142c6912315c02c9b498d49 Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Thu, 1 Oct 2026 13:30:00 +0000 Subject: [PATCH 3/3] [Support] Preserve trailing newlines in YAML block scalar output Output::blockScalarString always wrote "|", whose default clip chomping keeps exactly one trailing line break. A value without a trailing newline came back with one, and extra trailing newlines were dropped. Write "|-" or "|+" when the value does not end in exactly one newline. MIR modules and bodies end in exactly one newline, so their output does not change. Assisted-by: Claude --- llvm/lib/Support/YAMLTraits.cpp | 9 +++++++++ .../Remarks/YAMLRemarksSerializerTest.cpp | 9 ++++++--- llvm/unittests/Support/YAMLIOTest.cpp | 14 +++++++------- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/llvm/lib/Support/YAMLTraits.cpp b/llvm/lib/Support/YAMLTraits.cpp index 622806423635f..7c5ab1ef67712 100644 --- a/llvm/lib/Support/YAMLTraits.cpp +++ b/llvm/lib/Support/YAMLTraits.cpp @@ -756,6 +756,15 @@ void Output::blockScalarString(StringRef &S) { if (!StateStack.empty()) newLineCheck(); output(" |"); + // The default chomping keeps exactly one trailing line break, so strip or + // keep them explicitly to preserve the value. + if (!S.empty()) { + size_t LineBreaks = S.size() - S.rtrim('\n').size(); + if (LineBreaks == 0) + output("-"); + else if (LineBreaks > 1) + output("+"); + } unsigned Indent = StateStack.empty() ? 1 : StateStack.size(); diff --git a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp index 53ca5d2fe2562..1950c01dfb034 100644 --- a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp +++ b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp @@ -214,7 +214,7 @@ TEST(YAMLRemarks, SerializerRemarkMultiLineArg) { "Name: name\n" "Function: func\n" "Args:\n" - " - block: |\n" + " - block: |-\n" " abc\n" " def\n" " ghi\n" @@ -223,9 +223,12 @@ TEST(YAMLRemarks, SerializerRemarkMultiLineArg) { } TEST(YAMLRemarks, SerializerRemarkRoundTrip) { - // Values that the literal block form cannot hold must survive a round trip. + // Values must survive a round trip, whether they are written as a block or + // as a quoted scalar. StringRef Vals[] = {"abc\ndef\n\x01ghi", "abc\r\ndef\r\nghi", - " abc\ndef\nghi", "abc\ndef\nghi\n"}; + " abc\ndef\nghi", "abc\ndef\nghi", + "abc\ndef\nghi\n", "abc\ndef\nghi\n\n", + "\nabc\ndef\nghi"}; remarks::Remark R; R.RemarkType = remarks::Type::Missed; R.PassName = "pass"; diff --git a/llvm/unittests/Support/YAMLIOTest.cpp b/llvm/unittests/Support/YAMLIOTest.cpp index 78e19c5ff4f2c..216ee353555fc 100644 --- a/llvm/unittests/Support/YAMLIOTest.cpp +++ b/llvm/unittests/Support/YAMLIOTest.cpp @@ -1211,12 +1211,12 @@ TEST(YAMLIO, TestReadWriteMultilineStringType) { yin >> map2; EXPECT_FALSE(yin.error()); - EXPECT_EQ(map2.name.str, "An Item\n"); - EXPECT_EQ(map2.description.str, "Hello\nWorld\n"); + EXPECT_EQ(map2.name.str, "An Item"); + EXPECT_EQ(map2.description.str, "Hello\nWorld"); EXPECT_EQ(map2.ingredients.str, "SubItem 1\nSub Item 2\n\nSub Item 3\n"); - EXPECT_EQ(map2.recipes.str, "\n\nTest 1\n"); + EXPECT_EQ(map2.recipes.str, "\n\nTest 1\n\n\n"); EXPECT_TRUE(map2.warningLabels.str.empty()); - EXPECT_TRUE(map2.documentation.str.empty()); + EXPECT_EQ(map2.documentation.str, "\n\n"); EXPECT_EQ(map2.price, 350); } } @@ -1248,7 +1248,7 @@ TEST(YAMLIO, TestReadWriteBlockScalarDocuments) { EXPECT_FALSE(yin.error()); EXPECT_EQ(documents2.size(), size_t(1)); - EXPECT_EQ(documents2[0].str, "Hello\nWorld\n"); + EXPECT_EQ(documents2[0].str, "Hello\nWorld"); } } @@ -1268,7 +1268,7 @@ TEST(YAMLIO, TestReadWriteBlockScalarValue) { yin >> doc; EXPECT_FALSE(yin.error()); - EXPECT_EQ(doc.str, "Just a block\nscalar doc\n"); + EXPECT_EQ(doc.str, "Just a block\nscalar doc"); } } @@ -1294,7 +1294,7 @@ TEST(YAMLIO, TestScalarAfterBlockScalar) { Output yout(ostr); yout << v; EXPECT_EQ(output, R"(--- -- block_scalac: | +- block_scalac: |- AA BB scalar: a _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
