[MsgPack] Handle Expected<T> errors in document reader #73183
Merged
Conversation
This file contains hidden or 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
This was causing an assert on invalid in the modified test case.
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-binary-utilities Author: Emma Pilkington (epilk) ChangesThis was causing an assert on invalid in the modified test case. Fixes this Issue: #48378 Thanks for taking a look! Full diff: https://github.com/llvm/llvm-project/pull/73183.diff 5 Files Affected:
diff --git a/llvm/include/llvm/BinaryFormat/MsgPackReader.h b/llvm/include/llvm/BinaryFormat/MsgPackReader.h
index 5123fb65cf0951b..29bab8f9691afa9 100644
--- a/llvm/include/llvm/BinaryFormat/MsgPackReader.h
+++ b/llvm/include/llvm/BinaryFormat/MsgPackReader.h
@@ -18,7 +18,12 @@
/// msgpack::Reader MPReader(input);
/// msgpack::Object Obj;
///
-/// while (MPReader.read(Obj)) {
+/// while (true) {
+/// Expected<bool> ReadObj = MPReader.read(&Obj);
+/// if (!ReadObj)
+/// // Handle error...
+/// if (!ReadObj.get())
+/// break; // Reached end of input
/// switch (Obj.Kind) {
/// case msgpack::Type::Int:
// // Use Obj.Int
diff --git a/llvm/lib/BinaryFormat/MsgPackDocument.cpp b/llvm/lib/BinaryFormat/MsgPackDocument.cpp
index 21ffa35dfb6eeb8..11598ee24d6f21c 100644
--- a/llvm/lib/BinaryFormat/MsgPackDocument.cpp
+++ b/llvm/lib/BinaryFormat/MsgPackDocument.cpp
@@ -143,7 +143,13 @@ bool Document::readFromBlob(
// On to next element (or key if doing a map key next).
// Read the value.
Object Obj;
- if (!MPReader.read(Obj)) {
+ Expected<bool> ReadObj = MPReader.read(Obj);
+ if (!ReadObj) {
+ // FIXME: Propagate the Error to the caller.
+ consumeError(ReadObj.takeError());
+ return false;
+ }
+ if (!ReadObj.get()) {
if (Multi && Stack.size() == 1) {
// OK to finish here as we've just done a top-level element with Multi
break;
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp b/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
index 48d0bde139d7728..0fa67c559cb2916 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
@@ -83,7 +83,6 @@ bool AMDGPUPALMetadata::setFromLegacyBlob(StringRef Blob) {
// Set PAL metadata from msgpack blob.
bool AMDGPUPALMetadata::setFromMsgPackBlob(StringRef Blob) {
- msgpack::Reader Reader(Blob);
return MsgPackDoc.readFromBlob(Blob, /*Multi=*/false);
}
diff --git a/llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v3.test b/llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v3.test
index b6713377d7ef4eb..dd090b9483e2901 100644
--- a/llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v3.test
+++ b/llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v3.test
@@ -9,16 +9,15 @@
# LLVM-NEXT: NoteSection {
# LLVM-NEXT: Name: .note.nt_amdgpu_metadata
# LLVM-NEXT: Offset: 0x40
-# LLVM-NEXT: Size: 0x28
+# LLVM-NEXT: Size: 0x38
# LLVM-NEXT: Note {
# LLVM-NEXT: Owner: AMDGPU
-# LLVM-NEXT: Data size: 0x11
+# LLVM-NEXT: Data size: 0x24
# LLVM-NEXT: Type: NT_AMDGPU_METADATA (AMDGPU Metadata)
# LLVM-NEXT: AMDGPU Metadata: Invalid AMDGPU Metadata
# LLVM-NEXT: ---
-# LLVM-NEXT: 0: 0
-# LLVM-NEXT: amdhsa.kernels:
-# LLVM-NEXT: - 0
+# LLVM-NEXT: amdhsa.kernels:
+# LLVM-NEXT: - .name: test_kernel
# LLVM-NEXT: ...
# LLVM-EMPTY:
# LLVM-NEXT: }
@@ -27,13 +26,12 @@
# GNU: Displaying notes found in: .note.nt_amdgpu_metadata
# GNU-NEXT: Owner Data size Description
-# GNU-NEXT: AMDGPU 0x00000011 NT_AMDGPU_METADATA (AMDGPU Metadata)
+# GNU-NEXT: AMDGPU 0x00000024 NT_AMDGPU_METADATA (AMDGPU Metadata)
# GNU-NEXT: AMDGPU Metadata:
# GNU-NEXT: Invalid AMDGPU Metadata
# GNU-NEXT: ---
-# GNU-NEXT: 0: 0
# GNU-NEXT: amdhsa.kernels:
-# GNU-NEXT: - 0
+# GNU-NEXT: - .name: test_kernel
# GNU-NEXT: ...
--- !ELF
@@ -48,4 +46,4 @@ Sections:
- Name: AMDGPU
Type: NT_AMDGPU_METADATA
## Desc contains 'amdhsa.kernels' without valid entries.
- Desc: '82ae616d646873612e6b65726e656c7391'
+ Desc: '81ae616d646873612e6b65726e656c739181a52e6e616d65ab746573745f6b65726e656c'
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 25e0d3fc6ab1cce..0ed791c30954e8d 100644
--- a/llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s
+++ b/llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s
@@ -26,6 +26,8 @@
# GNU-NEXT: Owner Data size Description
# GNU-NEXT: AMDGPU 0x00000003 NT_AMDGPU_METADATA (AMDGPU Metadata)
# GNU-NEXT: description data: 12 34 56
+# GNU-NEXT: AMDGPU 0x00000003 NT_AMDGPU_METADATA (AMDGPU Metadata)
+# GNU-NEXT: description data: ab cd ef
# GNU-EMPTY:
# LLVM: Notes [
@@ -57,7 +59,7 @@
# LLVM-NEXT: NoteSection {
# LLVM-NEXT: Name: .note.bar
# LLVM-NEXT: Offset: 0x128
-# LLVM-NEXT: Size: 0x18
+# LLVM-NEXT: Size: 0x30
# LLVM-NEXT: Note {
# LLVM-NEXT: Owner: AMDGPU
# LLVM-NEXT: Data size: 0x3
@@ -66,6 +68,14 @@
# LLVM-NEXT: 0000: 123456 |.4V|
# LLVM-NEXT: )
# LLVM-NEXT: }
+# LLVM-NEXT: Note {
+# LLVM-NEXT: Owner: AMDGPU
+# LLVM-NEXT: Data size: 0x3
+# LLVM-NEXT: Type: NT_AMDGPU_METADATA (AMDGPU Metadata)
+# LLVM-NEXT: Description data (
+# LLVM-NEXT: 0000: ABCDEF |...|
+# LLVM-NEXT: )
+# LLVM-NEXT: }
# LLVM-NEXT: }
# LLVM-NEXT:]
@@ -87,7 +97,6 @@ Sections:
- Name: AMDGPU
Type: NT_AMDGPU_METADATA
Desc: '123456'
- # TODO: https://bugs.llvm.org/show_bug.cgi?id=49034
- # - Name: AMDGPU
- # Type: NT_AMDGPU_METADATA
- # Desc: 'abcdef'
+ - Name: AMDGPU
+ Type: NT_AMDGPU_METADATA
+ Desc: 'abcdef'
|
arichardson
approved these changes
Nov 27, 2023
| if (!MPReader.read(Obj)) { | ||
| Expected<bool> ReadObj = MPReader.read(Obj); | ||
| if (!ReadObj) { | ||
| // FIXME: Propagate the Error to the caller. |
There was a problem hiding this comment.
Do you have plans to address this in a follow-up change?
There was a problem hiding this comment.
I wasn't planning on it to be honest, it seems like the callers of this function aren't particularly interested in the failure case (e.g. https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-readobj/ELFDumper.cpp#L5527). But now that you mention it, maybe they should be. I'll write up a patch to report these errors later this week.
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 was causing an assert on invalid in the modified test case.
Fixes this Issue: #48378
Thanks for taking a look!