[llvm-branch-commits] [llvm] [Remarks][Support] Make YAML remark arguments round-trip (PR #227654)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Thu Oct 1 06:35:00 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-support
Author: Henrich Lauko (xlauko)
<details>
<summary>Changes</summary>
The YAML remark serializer writes an argument value with more than one newline as a literal block scalar. Block scalars have no escapes, so if the value also contains a control character other than tab or newline, the raw byte ends up in the file. For `"abc\ndef\n\x01ghi"`, PyYAML rejects the document with `unacceptable character #x0001`, while `llvm-remarkutil` accepts it without complaint. Values like that now go through the double-quoted path, which escapes them. Ordinary multi-line values keep the block form. Two more cases also fall back to the quoted form:
- A carriage return, since a reader turns a CR inside a block scalar into a line break.
- A first line that starts with a space. Readers take the block's indentation from that line, so `" abc\ndef\nghi"` used to produce a document that neither LLVM's parser nor PyYAML could read.
The parser had the matching problem on the way back in. `parseStr` took the raw scalar text and stripped only single quotes, so a double-quoted value came back with its quotes and escapes, and `'it''s'` came back as `it''s`. It now decodes scalars with `ScalarNode::getValue`. Unescaped values are copied into an allocator owned by the parser. Plain values still point into the input buffer.
A bad escape such as `"a\qb"` now fails the parse instead of silently becoming an empty string. So does a value that decodes to a null character. The bitstream string table stores NUL-terminated strings, so a NUL would shift every later string to the wrong index.
`Output::blockScalarString` always wrote `|`, which keeps exactly one trailing newline. Block values without one came back with one, and extra trailing newlines were dropped. It now writes `|-` or `|+` when needed. MIR output does not change, since printed modules and bodies end in exactly one newline.
Stacked on #<!-- -->228072, which fixes the use-after-free of block scalar values.
Assisted-by: Claude Code (Claude Fable 5.1).
---
Full diff: https://github.com/llvm/llvm-project/pull/227654.diff
8 Files Affected:
- (modified) llvm/lib/Remarks/YAMLRemarkParser.cpp (+18-18)
- (modified) llvm/lib/Remarks/YAMLRemarkParser.h (+2-2)
- (modified) llvm/lib/Remarks/YAMLRemarkSerializer.cpp (+15-1)
- (modified) llvm/lib/Support/YAMLTraits.cpp (+9)
- (added) llvm/test/tools/llvm-remarkutil/yaml2bitstream-null.test (+12)
- (modified) llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp (+51)
- (modified) llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp (+63)
- (modified) llvm/unittests/Support/YAMLIOTest.cpp (+7-7)
``````````diff
diff --git a/llvm/lib/Remarks/YAMLRemarkParser.cpp b/llvm/lib/Remarks/YAMLRemarkParser.cpp
index 405575b3123de..31d255b56d55b 100644
--- a/llvm/lib/Remarks/YAMLRemarkParser.cpp
+++ b/llvm/lib/Remarks/YAMLRemarkParser.cpp
@@ -265,24 +265,24 @@ 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);
- // The block value lives in the YAML document, which next() frees before
- // returning the remark.
- Result = ValueBlock->getValue().copy(Alloc);
- } 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);
+ // 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);
}
Expected<unsigned> YAMLRemarkParser::parseUnsigned(yaml::KeyValueNode &Node) {
diff --git a/llvm/lib/Remarks/YAMLRemarkParser.h b/llvm/lib/Remarks/YAMLRemarkParser.h
index e53adf1bd16e8..7de905b2a5bf8 100644
--- a/llvm/lib/Remarks/YAMLRemarkParser.h
+++ b/llvm/lib/Remarks/YAMLRemarkParser.h
@@ -59,8 +59,8 @@ 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 block scalar values, which live in the YAML document that
- /// next() frees.
+ /// 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/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/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 95ca63fc979f9..f74c9958e0ff3 100644
--- a/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp
+++ b/llvm/unittests/Remarks/YAMLRemarksParsingTest.cpp
@@ -384,6 +384,27 @@ 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"));
+ // 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) {
@@ -512,6 +533,36 @@ TEST(YAMLRemarks, ContentsBlockScalar) {
EXPECT_EQ(checkStr(Remark.Args[0].Val, 8), "abc\ndef\n");
}
+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"
+ "\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);
+
+ 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..1950c01dfb034 100644
--- a/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp
+++ b/llvm/unittests/Remarks/YAMLRemarksSerializerTest.cpp
@@ -193,3 +193,66 @@ 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 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",
+ "abc\ndef\nghi\n", "abc\ndef\nghi\n\n",
+ "\nabc\ndef\nghi"};
+ 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);
+}
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
``````````
</details>
https://github.com/llvm/llvm-project/pull/227654
More information about the llvm-branch-commits
mailing list