[llvm] [MsgPack] Handle Expected<T> errors in document reader (PR #73183)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 22 14:55:47 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-binary-utilities
Author: Emma Pilkington (epilk)
<details>
<summary>Changes</summary>
This was causing an assert on invalid in the modified test case.
Fixes this Issue: https://github.com/llvm/llvm-project/issues/48378
Thanks for taking a look!
---
Full diff: https://github.com/llvm/llvm-project/pull/73183.diff
5 Files Affected:
- (modified) llvm/include/llvm/BinaryFormat/MsgPackReader.h (+6-1)
- (modified) llvm/lib/BinaryFormat/MsgPackDocument.cpp (+7-1)
- (modified) llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp (-1)
- (modified) llvm/test/tools/llvm-readobj/ELF/note-amd-invalid-v3.test (+7-9)
- (modified) llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s (+14-5)
``````````diff
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'
``````````
</details>
https://github.com/llvm/llvm-project/pull/73183
More information about the llvm-commits
mailing list