Skip to content

[MsgPack] Return an Error instead of bool from readFromBlob #74357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
epilk wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[MsgPack] Return an Error instead of bool from readFromBlob #74357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38d421a
[MsgPack] Return an Error instead of bool from readFromBlob
epilk Dec 4, 2023
e524747
Address review comments
epilk Dec 5, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/include/llvm/BinaryFormat/MsgPackDocument.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ class Document {
/// into the original blob). 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.
/// an error if failed due to illegal format or merge error.
///
/// The Merger arg is a callback function that is called when the merge has a
/// conflict, that is, it is trying to set an item that is already set. If the
Expand All @@ -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) {
Expand Down
30 changes: 17 additions & 13 deletions llvm/lib/BinaryFormat/MsgPackDocument.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -187,7 +186,9 @@ bool Document::readFromBlob(
Node = getArrayNode();
break;
default:
return false; // Raw and Extension not supported
return make_error<StringError>(
"Unsupported msgpack object type",
std::make_error_code(std::errc::invalid_argument));
}

// Store it.
Expand Down Expand Up @@ -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
Expand All @@ -246,7 +250,7 @@ bool Document::readFromBlob(
Stack.pop_back();
}
} while (!Stack.empty());
return true;
return Error::success();
}

struct WriterStackLevel {
Expand Down
20 changes: 10 additions & 10 deletions llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,27 +63,27 @@ 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);
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));
Copy link
Contributor

@slinder1 slinder1 Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Member Author

@epilk epilk Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

@slinder1 slinder1 Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Contributor

@arsenm arsenm Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

// Given the calling convention, calculate the register number for rsrc1. In
Expand Down
11 changes: 5 additions & 6 deletions llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
};
Expand Down
6 changes: 6 additions & 0 deletions llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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: )
Expand All @@ -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: )
Expand Down
41 changes: 23 additions & 18 deletions llvm/tools/llvm-readobj/ELFDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

@arichardson arichardson Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should return Expected instead of adding the extra boolean field?
Copy link
Member Author

@epilk epilk Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

@arichardson arichardson Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Member

@arichardson arichardson Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(); }
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};
}
}
}
Expand Down Expand Up @@ -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();
if (!N.IsError)
return Error::success();
// Fallthrough to printing the description data blob on error.
}
} else if (Name == "LLVMOMPOFFLOAD") {
if (printLLVMOMPOFFLOADNote<ELFT>(OS, Type, Descriptor))
Expand Down Expand Up @@ -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();
if (!N.IsError)
return Error::success();
// Fallthrough to printing the description data blob on error.
}
} else if (Name == "LLVMOMPOFFLOAD") {
if (printLLVMOMPOFFLOADNoteLLVMStyle<ELFT>(Type, Descriptor, W))
Expand Down
Loading