labath added a comment.
Thanks. IIUC, all the existing tests just cover the yaml2obj direction. Could
you add something for the other direction too? Maybe add an exception stream to
`test/tools/obj2yaml/basic-minidump.yaml`?
================
Comment at: llvm/include/llvm/ObjectYAML/MinidumpYAML.h:162-181
+/// ExceptionStream minidump stream.
+struct ExceptionStream : public Stream {
+ minidump::ExceptionStream MDExceptionStream;
+ yaml::BinaryRef ThreadContext;
+
+ explicit ExceptionStream(const minidump::ExceptionStream &MDExceptionStream,
+ ArrayRef<uint8_t> ThreadContext)
----------------
I've been trying to keep this somewhat sorted. Could you move this before the
`MemoryInfoListStream` class? Also, in the previous patch we've moved the
default constructors to front. It would be good to make this consistent with
that.
================
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:394
+ mapOptionalHex(IO, "Exception Address", Exception.ExceptionAddress, 0);
+ IO.mapOptional("Number Parameters", Exception.NumberParameters,
+ support::ulittle32_t(0u));
----------------
This file has a helper function for this (`mapOptional(IO, "name", value, 0)`.
I'd consider changing the field name to "Number of Parameters" even though it
does not match the field name, as it reads weird without that. I'm not sure why
the microsoft naming is inconsistent here -- most of the other minidump structs
have "of" in their name already (BaseOfImage, SizeOfImage, etc.), but at least
we can be consistent.
================
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:408-414
+StringRef yaml::MappingTraits<minidump::Exception>::validate(
+ yaml::IO &IO, minidump::Exception &Exception) {
+ if (Exception.NumberParameters > Exception::MaxParameters)
+ return "Exception reports too many parameters";
+ return "";
+}
+
----------------
Could you remove this bit too? While it is technically invalid, this is not
something that yaml2obj needs to care about (as it does not prevent successful
serialization), and it would be nice to be able to use it to generate a test
case with an invalid number (because that is something lldb should care about
and expect/handle)..
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68657/new/
https://reviews.llvm.org/D68657
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
