[MsgPack] Return an Error instead of bool from readFromBlob #74357
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-binary-utilities Author: Emma Pilkington (epilk) ChangesThis is a follow-up to: #73183 Full diff: https://github.com/llvm/llvm-project/pull/74357.diff 7 Files Affected:
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;
|
slinder1
requested changes
Dec 4, 2023
| return MsgPackDoc.readFromBlob(Blob, /*Multi=*/false); | ||
| void AMDGPUPALMetadata::setFromMsgPackBlob(StringRef Blob) { | ||
| if (!Blob.empty()) | ||
| handleAllErrors(MsgPackDoc.readFromBlob(Blob, /*Multi=*/false)); |
There was a problem hiding this comment.
I'm not sure I follow why this buries the Error here; is this another "TODO" or is there really a guarantee it cannot fail?
There was a problem hiding this comment.
I guess it's an assertion that the Blob is valid. There should never be an error so long as the frontend* generates valid metadata. I don't think its possible to fail more gracefully here either, our options are either 1) discard and overwrite the metadata, or 2) don't make any amendments to it. Previously we were doing 1) implicitly, which seems wrong to me, but 2) also seems wrong.
* What frontend am I talking about? That's not rhetorical, I don't know! Clang doesn't seem to be aware of "!amdgpu.pal.metadata", but this comment references a frontend: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp#L29 . This metadata is used by lit tests.
There was a problem hiding this comment.
It might be worth updating that comment, I agree "the frontend" is a little opaque here! I believe it refers to LLPC: https://github.com/GPUOpen-Drivers/llpc
For example: https://github.com/GPUOpen-Drivers/llpc/blob/1005e28c078e07fc0b83363260811f733edaeafd/lgc/state/PalMetadata.cpp#L123 and https://github.com/GPUOpen-Drivers/llpc/blob/1005e28c078e07fc0b83363260811f733edaeafd/lgc/include/lgc/state/AbiMetadata.h#L636
In general we don't want to actually assert for any parseable IR/bitcode input, we would rather exit (somewhat) gracefully with an intelligible error, even when not built with asserts enabled.
I can't seem to find where this method is actually used, though. @trenouf do you know where the AMDGPUPALMetadata::setFrom.*Blob methods are called? Should they propagate errors to the caller? Or is it OK to report_fatal_error it here?
There was a problem hiding this comment.
We should never assert on invalid input. The only way an assert would be OK is if the IR verifier rejected any malformed metadata which could reach here
arichardson
reviewed
Dec 4, 2023
| @@ -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) { | |||
There was a problem hiding this comment.
Maybe this should return Expected instead of adding the extra boolean field?
There was a problem hiding this comment.
This isn't really a "I'm returning either an Error or a valid AMDGPUNote" situation. In the error case, the Error object gets written into the std::string fields, I just wanted to indicate to the caller that there was an error pretty-printing so it would still dump the binary blob, which seems useful to include.
There was a problem hiding this comment.
This should still be possible if you return an Expected<> object? We can just print the error string from the bad Expected<> and then continue to the raw dumping?
There was a problem hiding this comment.
Something like
const Expected<AMDGPUNote> N = getAMDGPUNote<ELFT>(Type, Descriptor);
if (!N) {
OS << " AMDGPU Metadata:\n " <<toString(N.takeError())) << '\n';
// Fallthrough to printing the description data blob on error.
} else {
OS << " " << N->Type << ":\n " << N->Value << '\n';
return Error::success();
}
arsenm
approved these changes
Feb 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to: #73183