[llvm] [MsgPack] Return an Error instead of bool from readFromBlob (PR #74357)
Emma Pilkington via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 4 11:04:36 PST 2023
https://github.com/epilk created https://github.com/llvm/llvm-project/pull/74357
This is a follow-up to: https://github.com/llvm/llvm-project/pull/73183
>From 38d421a8d6c042a4a28a4136ed306d4e9dd2c171 Mon Sep 17 00:00:00 2001
From: Emma Pilkington <emma.pilkington95 at gmail.com>
Date: Mon, 4 Dec 2023 13:13:45 -0500
Subject: [PATCH] [MsgPack] Return an Error instead of bool from readFromBlob
---
.../llvm/BinaryFormat/MsgPackDocument.h | 2 +-
llvm/lib/BinaryFormat/MsgPackDocument.cpp | 30 +++----
.../Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp | 23 +++---
.../Target/AMDGPU/Utils/AMDGPUPALMetadata.h | 11 ++-
.../llvm-readobj/ELF/note-amdgpu-invalid.s | 6 ++
llvm/tools/llvm-readobj/ELFDumper.cpp | 41 +++++-----
.../BinaryFormat/MsgPackDocumentTest.cpp | 78 ++++++++++---------
7 files changed, 107 insertions(+), 84 deletions(-)
diff --git a/llvm/include/llvm/BinaryFormat/MsgPackDocument.h b/llvm/include/llvm/BinaryFormat/MsgPackDocument.h
index 7a181bd9bf841..680df75f81743 100644
--- a/llvm/include/llvm/BinaryFormat/MsgPackDocument.h
+++ b/llvm/include/llvm/BinaryFormat/MsgPackDocument.h
@@ -424,7 +424,7 @@ class Document {
/// map entry, a nil node otherwise.
///
/// The default for Merger is to disallow any conflict.
- bool readFromBlob(
+ Error readFromBlob(
StringRef Blob, bool Multi,
function_ref<int(DocNode *DestNode, DocNode SrcNode, DocNode MapKey)>
Merger = [](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
diff --git a/llvm/lib/BinaryFormat/MsgPackDocument.cpp b/llvm/lib/BinaryFormat/MsgPackDocument.cpp
index 11598ee24d6f2..ed4758757560f 100644
--- a/llvm/lib/BinaryFormat/MsgPackDocument.cpp
+++ b/llvm/lib/BinaryFormat/MsgPackDocument.cpp
@@ -126,9 +126,9 @@ struct StackLevel {
// If Multi, then this sets root to an array and adds top-level objects to it.
// If !Multi, then it only reads a single top-level object, even if there are
// more, and sets root to that.
-// Returns false if failed due to illegal format or merge error.
-
-bool Document::readFromBlob(
+// Returns an error if failed due to illegal format or merge error, or
+// Error::success() if succeeded.
+Error Document::readFromBlob(
StringRef Blob, bool Multi,
function_ref<int(DocNode *DestNode, DocNode SrcNode, DocNode MapKey)>
Merger) {
@@ -144,17 +144,16 @@ bool Document::readFromBlob(
// Read the value.
Object Obj;
Expected<bool> ReadObj = MPReader.read(Obj);
- if (!ReadObj) {
- // FIXME: Propagate the Error to the caller.
- consumeError(ReadObj.takeError());
- return false;
- }
+ if (!ReadObj)
+ return ReadObj.takeError();
if (!ReadObj.get()) {
if (Multi && Stack.size() == 1) {
// OK to finish here as we've just done a top-level element with Multi
break;
}
- return false; // Finished too early
+ return make_error<StringError>(
+ "Unexpected end of document",
+ std::make_error_code(std::errc::invalid_argument));
}
// Convert it into a DocNode.
DocNode Node;
@@ -187,7 +186,9 @@ bool Document::readFromBlob(
Node = getArrayNode();
break;
default:
- return false; // Raw and Extension not supported
+ return make_error<StringError>(
+ "Raw and Extension not supported",
+ std::make_error_code(std::errc::invalid_argument));
}
// Store it.
@@ -220,8 +221,11 @@ bool Document::readFromBlob(
? Stack.back().MapKey
: getNode();
MergeResult = Merger(DestNode, Node, MapKey);
- if (MergeResult < 0)
- return false; // Merge conflict resolution failed
+ if (MergeResult < 0) {
+ return make_error<StringError>(
+ "Merge conflict resolution failed",
+ std::make_error_code(std::errc::invalid_argument));
+ }
assert(!((Node.isMap() && !DestNode->isMap()) ||
(Node.isArray() && !DestNode->isArray())));
} else
@@ -246,7 +250,7 @@ bool Document::readFromBlob(
Stack.pop_back();
}
} while (!Stack.empty());
- return true;
+ return Error::success();
}
struct WriterStackLevel {
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp b/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
index 0fa67c559cb29..cdc2693f3a299 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
@@ -63,27 +63,28 @@ void AMDGPUPALMetadata::readFromIR(Module &M) {
}
}
-// Set PAL metadata from a binary blob from the applicable .note record.
-// Returns false if bad format. Blob must remain valid for the lifetime of the
-// Metadata.
-bool AMDGPUPALMetadata::setFromBlob(unsigned Type, StringRef Blob) {
+// Set PAL metadata from a binary blob from the applicable .note record. Blob
+// must remain valid for the lifetime of the Metadata.
+void AMDGPUPALMetadata::setFromBlob(unsigned Type, StringRef Blob) {
BlobType = Type;
- if (Type == ELF::NT_AMD_PAL_METADATA)
- return setFromLegacyBlob(Blob);
- return setFromMsgPackBlob(Blob);
+ if (Type == ELF::NT_AMD_PAL_METADATA) {
+ setFromLegacyBlob(Blob);
+ } else {
+ setFromMsgPackBlob(Blob);
+ }
}
// Set PAL metadata from legacy (array of key=value pairs) blob.
-bool AMDGPUPALMetadata::setFromLegacyBlob(StringRef Blob) {
+void AMDGPUPALMetadata::setFromLegacyBlob(StringRef Blob) {
auto Data = reinterpret_cast<const uint32_t *>(Blob.data());
for (unsigned I = 0; I != Blob.size() / sizeof(uint32_t) / 2; ++I)
setRegister(Data[I * 2], Data[I * 2 + 1]);
- return true;
}
// Set PAL metadata from msgpack blob.
-bool AMDGPUPALMetadata::setFromMsgPackBlob(StringRef Blob) {
- return MsgPackDoc.readFromBlob(Blob, /*Multi=*/false);
+void AMDGPUPALMetadata::setFromMsgPackBlob(StringRef Blob) {
+ if (!Blob.empty())
+ handleAllErrors(MsgPackDoc.readFromBlob(Blob, /*Multi=*/false));
}
// Given the calling convention, calculate the register number for rsrc1. In
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h b/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
index 158f766d04854..ad20a609e57c7 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
@@ -37,10 +37,9 @@ class AMDGPUPALMetadata {
// per-function modification.
void readFromIR(Module &M);
- // Set PAL metadata from a binary blob from the applicable .note record.
- // Returns false if bad format. Blob must remain valid for the lifetime of
- // the Metadata.
- bool setFromBlob(unsigned Type, StringRef Blob);
+ // Set PAL metadata from a binary blob from the applicable .note record. Blob
+ // must remain valid for the lifetime of the Metadata.
+ void setFromBlob(unsigned Type, StringRef Blob);
// Set the rsrc1 register in the metadata for a particular shader stage.
// In fact this ORs the value into any previous setting of the register.
@@ -198,8 +197,8 @@ class AMDGPUPALMetadata {
// helper for the public wrapper functions that request Major or Minor
unsigned getPALVersion(unsigned idx);
- bool setFromLegacyBlob(StringRef Blob);
- bool setFromMsgPackBlob(StringRef Blob);
+ void setFromLegacyBlob(StringRef Blob);
+ void setFromMsgPackBlob(StringRef Blob);
void toLegacyBlob(std::string &Blob);
void toMsgPackBlob(std::string &Blob);
};
diff --git a/llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s b/llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s
index 0ed791c30954e..26cf621638d09 100644
--- a/llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s
+++ b/llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s
@@ -25,8 +25,12 @@
# GNU-NEXT: Displaying notes found in: .note.bar
# GNU-NEXT: Owner Data size Description
# GNU-NEXT: AMDGPU 0x00000003 NT_AMDGPU_METADATA (AMDGPU Metadata)
+# GNU-NEXT: AMDGPU Metadata:
+# GNU-NEXT: Invalid AMDGPU Metadata
# GNU-NEXT: description data: 12 34 56
# GNU-NEXT: AMDGPU 0x00000003 NT_AMDGPU_METADATA (AMDGPU Metadata)
+# GNU-NEXT: AMDGPU Metadata:
+# GNU-NEXT: Invalid Raw with insufficient payload
# GNU-NEXT: description data: ab cd ef
# GNU-EMPTY:
@@ -64,6 +68,7 @@
# LLVM-NEXT: Owner: AMDGPU
# LLVM-NEXT: Data size: 0x3
# LLVM-NEXT: Type: NT_AMDGPU_METADATA (AMDGPU Metadata)
+# LLVM-NEXT: AMDGPU Metadata: Invalid AMDGPU Metadata
# LLVM-NEXT: Description data (
# LLVM-NEXT: 0000: 123456 |.4V|
# LLVM-NEXT: )
@@ -72,6 +77,7 @@
# LLVM-NEXT: Owner: AMDGPU
# LLVM-NEXT: Data size: 0x3
# LLVM-NEXT: Type: NT_AMDGPU_METADATA (AMDGPU Metadata)
+# LLVM-NEXT: AMDGPU Metadata: Invalid Raw with insufficient payload
# LLVM-NEXT: Description data (
# LLVM-NEXT: 0000: ABCDEF |...|
# LLVM-NEXT: )
diff --git a/llvm/tools/llvm-readobj/ELFDumper.cpp b/llvm/tools/llvm-readobj/ELFDumper.cpp
index d6d0ea35044ab..67a3d61a16a73 100644
--- a/llvm/tools/llvm-readobj/ELFDumper.cpp
+++ b/llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -5513,41 +5513,42 @@ static AMDNote getAMDNote(uint32_t NoteType, ArrayRef<uint8_t> Desc) {
struct AMDGPUNote {
std::string Type;
std::string Value;
+ bool IsError;
};
template <typename ELFT>
static AMDGPUNote getAMDGPUNote(uint32_t NoteType, ArrayRef<uint8_t> Desc) {
switch (NoteType) {
default:
- return {"", ""};
+ return {"", "", true};
case ELF::NT_AMDGPU_METADATA: {
StringRef MsgPackString =
StringRef(reinterpret_cast<const char *>(Desc.data()), Desc.size());
msgpack::Document MsgPackDoc;
- if (!MsgPackDoc.readFromBlob(MsgPackString, /*Multi=*/false))
- return {"", ""};
+ if (Error E = MsgPackDoc.readFromBlob(MsgPackString, /*Multi=*/false))
+ return {"AMDGPU Metadata", toString(std::move(E)), /*IsError=*/true};
+
+ if (MsgPackDoc.getRoot().isScalar()) {
+ // TODO: passing a scalar root to toYAML() asserts:
+ // (PolymorphicTraits<T>::getKind(Val) != NodeKind::Scalar &&
+ // "plain scalar documents are not supported")
+ // To avoid this crash we print the raw data instead.
+ return {"AMDGPU Metadata", "Invalid AMDGPU Metadata", /*IsError=*/true};
+ }
std::string MetadataString;
+ raw_string_ostream StrOS(MetadataString);
- // FIXME: Metadata Verifier only works with AMDHSA.
- // This is an ugly workaround to avoid the verifier for other MD
- // formats (e.g. amdpal)
+ // FIXME: Metadata Verifier only works with AMDHSA. This is an ugly
+ // workaround to avoid the verifier for other MD formats (e.g. amdpal)
if (MsgPackString.contains("amdhsa.")) {
AMDGPU::HSAMD::V3::MetadataVerifier Verifier(true);
if (!Verifier.verify(MsgPackDoc.getRoot()))
- MetadataString = "Invalid AMDGPU Metadata\n";
+ StrOS << "Invalid AMDGPU Metadata\n";
}
- raw_string_ostream StrOS(MetadataString);
- if (MsgPackDoc.getRoot().isScalar()) {
- // TODO: passing a scalar root to toYAML() asserts:
- // (PolymorphicTraits<T>::getKind(Val) != NodeKind::Scalar &&
- // "plain scalar documents are not supported")
- // To avoid this crash we print the raw data instead.
- return {"", ""};
- }
MsgPackDoc.toYAML(StrOS);
- return {"AMDGPU Metadata", StrOS.str()};
+ return {"AMDGPU Metadata", StrOS.str(), /*IsError=*/false};
}
}
}
@@ -5964,7 +5965,9 @@ template <class ELFT> void GNUELFDumper<ELFT>::printNotes() {
const AMDGPUNote N = getAMDGPUNote<ELFT>(Type, Descriptor);
if (!N.Type.empty()) {
OS << " " << N.Type << ":\n " << N.Value << '\n';
- return Error::success();
+ // Fallthrough to printing the description data blob on error.
+ if (!N.IsError)
+ return Error::success();
}
} else if (Name == "LLVMOMPOFFLOAD") {
if (printLLVMOMPOFFLOADNote<ELFT>(OS, Type, Descriptor))
@@ -7661,7 +7664,9 @@ template <class ELFT> void LLVMELFDumper<ELFT>::printNotes() {
const AMDGPUNote N = getAMDGPUNote<ELFT>(Type, Descriptor);
if (!N.Type.empty()) {
W.printString(N.Type, N.Value);
- return Error::success();
+ // Fallthrough to printing the description data blob on error.
+ if (!N.IsError)
+ return Error::success();
}
} else if (Name == "LLVMOMPOFFLOAD") {
if (printLLVMOMPOFFLOADNoteLLVMStyle<ELFT>(Type, Descriptor, W))
diff --git a/llvm/unittests/BinaryFormat/MsgPackDocumentTest.cpp b/llvm/unittests/BinaryFormat/MsgPackDocumentTest.cpp
index a8db0f1ad0cca..0450781ddad91 100644
--- a/llvm/unittests/BinaryFormat/MsgPackDocumentTest.cpp
+++ b/llvm/unittests/BinaryFormat/MsgPackDocumentTest.cpp
@@ -24,7 +24,7 @@ TEST(MsgPackDocument, DocNodeTest) {
TEST(MsgPackDocument, TestReadInt) {
Document Doc;
- bool Ok = Doc.readFromBlob(StringRef("\xd0\x00", 2), /*Multi=*/false);
+ bool Ok = !Doc.readFromBlob(StringRef("\xd0\x00", 2), /*Multi=*/false);
ASSERT_TRUE(Ok);
ASSERT_EQ(Doc.getRoot().getKind(), Type::Int);
ASSERT_EQ(Doc.getRoot().getInt(), 0);
@@ -33,8 +33,8 @@ TEST(MsgPackDocument, TestReadInt) {
TEST(MsgPackDocument, TestReadBinary) {
Document Doc;
uint8_t data[] = {1, 2, 3, 4};
- bool Ok =
- Doc.readFromBlob(StringRef("\xC4\x4\x1\x2\x3\x4", 6), /*Multi=*/false);
+ bool Ok = !Doc.readFromBlob(StringRef("\xC4\x4\x1\x2\x3\x4", 6),
+ /*Multi=*/false);
ASSERT_TRUE(Ok);
ASSERT_EQ(Doc.getRoot().getKind(), Type::Binary);
ASSERT_EQ(Doc.getRoot().getBinary().getBuffer(),
@@ -43,7 +43,7 @@ TEST(MsgPackDocument, TestReadBinary) {
TEST(MsgPackDocument, TestReadMergeArray) {
Document Doc;
- bool Ok = Doc.readFromBlob(StringRef("\x92\xd0\x01\xc0"), /*Multi=*/false);
+ bool Ok = !Doc.readFromBlob(StringRef("\x92\xd0\x01\xc0"), /*Multi=*/false);
ASSERT_TRUE(Ok);
ASSERT_EQ(Doc.getRoot().getKind(), Type::Array);
auto A = Doc.getRoot().getArray();
@@ -54,19 +54,19 @@ TEST(MsgPackDocument, TestReadMergeArray) {
auto SN = A[1];
ASSERT_EQ(SN.getKind(), Type::Nil);
- Ok = Doc.readFromBlob(StringRef("\x91\xd0\x2a"), /*Multi=*/false,
- [](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
- // Allow array, merging into existing elements, ORing
- // ints.
- if (DestNode->getKind() == Type::Int &&
- SrcNode.getKind() == Type::Int) {
- *DestNode = DestNode->getDocument()->getNode(
- DestNode->getInt() | SrcNode.getInt());
- return 0;
- }
- return DestNode->isArray() && SrcNode.isArray() ? 0
- : -1;
- });
+ Ok = !Doc.readFromBlob(
+ StringRef("\x91\xd0\x2a"), /*Multi=*/false,
+ [](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
+ // Allow array, merging into existing elements, ORing
+ // ints.
+ if (DestNode->getKind() == Type::Int &&
+ SrcNode.getKind() == Type::Int) {
+ *DestNode = DestNode->getDocument()->getNode(DestNode->getInt() |
+ SrcNode.getInt());
+ return 0;
+ }
+ return DestNode->isArray() && SrcNode.isArray() ? 0 : -1;
+ });
ASSERT_TRUE(Ok);
A = Doc.getRoot().getArray();
ASSERT_EQ(A.size(), 2u);
@@ -79,7 +79,7 @@ TEST(MsgPackDocument, TestReadMergeArray) {
TEST(MsgPackDocument, TestReadAppendArray) {
Document Doc;
- bool Ok = Doc.readFromBlob(StringRef("\x92\xd0\x01\xc0"), /*Multi=*/false);
+ bool Ok = !Doc.readFromBlob(StringRef("\x92\xd0\x01\xc0"), /*Multi=*/false);
ASSERT_TRUE(Ok);
ASSERT_EQ(Doc.getRoot().getKind(), Type::Array);
auto A = Doc.getRoot().getArray();
@@ -90,7 +90,8 @@ TEST(MsgPackDocument, TestReadAppendArray) {
auto SN = A[1];
ASSERT_EQ(SN.getKind(), Type::Nil);
- Ok = Doc.readFromBlob(StringRef("\x91\xd0\x2a"), /*Multi=*/false,
+ Ok =
+ !Doc.readFromBlob(StringRef("\x91\xd0\x2a"), /*Multi=*/false,
[](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
// Allow array, appending after existing elements
return DestNode->isArray() && SrcNode.isArray()
@@ -112,12 +113,12 @@ TEST(MsgPackDocument, TestReadAppendArray) {
TEST(MsgPackDocument, TestReadMergeMap) {
Document Doc;
- bool Ok = Doc.readFromBlob(StringRef("\x82\xa3"
- "foo"
- "\xd0\x01\xa3"
- "bar"
- "\xd0\x02"),
- /*Multi=*/false);
+ bool Ok = !Doc.readFromBlob(StringRef("\x82\xa3"
+ "foo"
+ "\xd0\x01\xa3"
+ "bar"
+ "\xd0\x02"),
+ /*Multi=*/false);
ASSERT_TRUE(Ok);
ASSERT_EQ(Doc.getRoot().getKind(), Type::Map);
auto M = Doc.getRoot().getMap();
@@ -129,15 +130,15 @@ TEST(MsgPackDocument, TestReadMergeMap) {
ASSERT_EQ(BarS.getKind(), Type::Int);
ASSERT_EQ(BarS.getInt(), 2);
- Ok = Doc.readFromBlob(StringRef("\x82\xa3"
- "foz"
- "\xd0\x03\xa3"
- "baz"
- "\xd0\x04"),
- /*Multi=*/false,
- [](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
- return DestNode->isMap() && SrcNode.isMap() ? 0 : -1;
- });
+ Ok = !Doc.readFromBlob(
+ StringRef("\x82\xa3"
+ "foz"
+ "\xd0\x03\xa3"
+ "baz"
+ "\xd0\x04"),
+ /*Multi=*/false, [](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
+ return DestNode->isMap() && SrcNode.isMap() ? 0 : -1;
+ });
ASSERT_TRUE(Ok);
ASSERT_EQ(M.size(), 4u);
FooS = M["foo"];
@@ -153,7 +154,7 @@ TEST(MsgPackDocument, TestReadMergeMap) {
ASSERT_EQ(BazS.getKind(), Type::Int);
ASSERT_EQ(BazS.getInt(), 4);
- Ok = Doc.readFromBlob(
+ Ok = !Doc.readFromBlob(
StringRef("\x82\xa3"
"foz"
"\xd0\x06\xa3"
@@ -192,6 +193,13 @@ TEST(MsgPackDocument, TestReadMergeMap) {
ASSERT_EQ(BayS.getInt(), 8);
}
+TEST(MsgPackDocument, TestReadInvalid) {
+ Document Doc;
+ Error E = Doc.readFromBlob(StringRef(), /*Multi=*/false);
+ ASSERT_TRUE((bool)E);
+ consumeError(std::move(E));
+}
+
TEST(MsgPackDocument, TestWriteInt) {
Document Doc;
Doc.getRoot() = 1;
More information about the llvm-commits
mailing list