Add ExceptionStream to llvm::Object::minidump

[JosephTremoulet]

Summary

This will allow updating MinidumpYAML and LLDB to use this common
definition.

[labath]

Thanks for taking your time to do this. I have one question: It looks like you're not using the exception code enum in the follow-up patch. I think that's completely reasonable given that the enum values are overloaded and system-dependent. But given this fact, and the fact that I am not convinced the enum values are completely right (e.g. the linux signal numbers depend also on the architecture -- though this may not manifest itself on the architectures that breakpad supports right now), what would you say to just dropping that enumeration?

[JosephTremoulet]

In D68656#1701658, @labath wrote:

Thanks for taking your time to do this. I have one question: It looks like you're not using the exception code enum in the follow-up patch. I think that's completely reasonable given that the enum values are overloaded and system-dependent. But given this fact, and the fact that I am not convinced the enum values are completely right (e.g. the linux signal numbers depend also on the architecture -- though this may not manifest itself on the architectures that breakpad supports right now), what would you say to just dropping that enumeration?

Yeah, I went back and forth myself on whether to include those. The thing is that I need to replace the definition of MinidumpException::DumpRequested with something in these types. When I did some research, I found that the breakpad constant in question is actually Linux-specific, and that there are similar constants defined for Mac and Windows. So I went the route of pulling in the big list of constants for each OS group. It's easy to miss, but in D68658 I actually do reference Linux_DumpRequested in code and in a comment make a reference Mac_Simulated and Windows_Simulated though not by name.

Perhaps a better approach would be to define just those three, with a comment about how the field gets used more generally and where external constants come from? I'll push an update that goes that route, please let me know what you think.

[JosephTremoulet]

• Remove the os-defined exception code enum values

[labath]

Hmm.. maybe just keep those constants defined in lldb somewhere (e.g. right next to or inside the RefreshStateAfterStop function)? That's the place which does the decoding and also includes the OS switch, so it makes it easier to explain their purpose and why they are different for each OS...

[JosephTremoulet]

• Remove ExceptionCode enumeration

[JosephTremoulet]

In D68656#1703711, @labath wrote:

Hmm.. maybe just keep those constants defined in lldb somewhere (e.g. right next to or inside the RefreshStateAfterStop function)? That's the place which does the decoding and also includes the OS switch, so it makes it easier to explain their purpose and why they are different for each OS...

That works for me. Updated.

[clayborg]

LGTM, Pavel?

[labath]

Looks fine, just a few nits inline.

llvm/include/llvm/BinaryFormat/Minidump.h

230

Delete or put a more meanigful comment here.

llvm/unittests/Object/MinidumpTest.cpp

756–758

Delete. The ASSERT_THAT_EXPECTED check should already print the error message if this fails. Was that not working for you for some reason?

[JosephTremoulet]

• Remove useless comment and leftover debugging cruft

[JosephTremoulet]

llvm/unittests/Object/MinidumpTest.cpp

756–758

Yeah, that does work as expected, this was leftover cruft from when I was misunderstanding the error message while writing the test, good catch.

llvm/include/llvm/BinaryFormat/Minidump.h

struct Thread {
   support::ulittle32_t PriorityClass;
   support::ulittle32_t Priority;
   support::ulittle64_t EnvironmentBlock;
   MemoryDescriptor Stack;
   LocationDescriptor Context;
 };
 static_assert(sizeof(Thread) == 48, "");
 
+struct Exception {
+  static constexpr size_t MaxParameters = 15;
+
+  support::ulittle32_t ExceptionCode;
+  support::ulittle32_t ExceptionFlags;
+  support::ulittle64_t ExceptionRecord;
+  support::ulittle64_t ExceptionAddress;
+  support::ulittle32_t NumberParameters;
+  support::ulittle32_t UnusedAlignment;
+  support::ulittle64_t ExceptionInformation[MaxParameters];
+};
+static_assert(sizeof(Exception) == 152, "");
+
+struct ExceptionStream {
+  support::ulittle32_t ThreadId;
+  support::ulittle32_t UnusedAlignment;
+  Exception ExceptionRecord;
+  LocationDescriptor ThreadContext;
+};
+static_assert(sizeof(ExceptionStream) == 168, "");
+
 } // namespace minidump
 
 template <> struct DenseMapInfo<minidump::StreamType> {
   static minidump::StreamType getEmptyKey() { return minidump::StreamType(-1); }
 
   static minidump::StreamType getTombstoneKey() {
     return minidump::StreamType(-2);
   }

llvm/include/llvm/Object/Minidump.h

public:
   /// returned if the file does not contain this stream, or if the stream is
   /// not large enough to contain the number of threads declared in the stream
   /// header. The consistency of the Thread entries themselves is not checked in
   /// any way.
   Expected<ArrayRef<minidump::Thread>> getThreadList() const {
     return getListStream<minidump::Thread>(minidump::StreamType::ThreadList);
   }
 
+  /// Returns the contents of the Exception stream.  An error is returned if the
+  /// file does not contain this stream, or the stream is smaller than the size
+  /// of the ExceptionStream structure.  The internal consistency of the stream
+  /// is not checked in any way.
+  Expected<const minidump::ExceptionStream &> getExceptionStream() const {
+    return getStream<minidump::ExceptionStream>(
+        minidump::StreamType::Exception);
+  }
+
   /// Returns the list of descriptors embedded in the MemoryList stream. The
   /// descriptors provide the content of interesting regions of memory at the
   /// time the minidump was taken. An error is returned if the file does not
   /// contain this stream, or if the stream is not large enough to contain the
   /// number of memory descriptors declared in the stream header. The
   /// consistency of the MemoryDescriptor entries themselves is not checked in
   /// any way.
   Expected<ArrayRef<minidump::MemoryDescriptor>> getMemoryList() const {

llvm/unittests/Object/MinidumpTest.cpp

TEST(MinidumpFile, getMemoryInfoList) {
   ASSERT_THAT_EXPECTED(ExpectedInfo, Succeeded());
   EXPECT_THAT(to_vector<3>(map_range(*ExpectedInfo,
                                      [](const MemoryInfo &Info) -> uint64_t {
                                        return Info.BaseAddress;
                                      })),
               testing::ElementsAre(0x0000000003020100u, 0x0000070605040000u,
                                    0x0001000908000000u));
 }
+
+TEST(MinidumpFile, getExceptionStream) {
+  std::vector<uint8_t> Data{
+      // Header
+      'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+      1, 0, 0, 0,                           // NumberOfStreams,
+      0x20, 0, 0, 0,                        // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
+      8, 9, 0, 1, 2, 3, 4, 5,               // Flags
+      // Stream Directory
+      6, 0, 0, 0, 168, 0, 0, 0, // Type, DataSize,
+      0x2c, 0, 0, 0,            // RVA
+      // Exception Stream
+      1, 2, 3, 4, // Thread ID
+      0, 0, 0, 0, // Padding
+      // Exception Record
+      2, 3, 4, 2, 7, 8, 8, 9,  // Code, Flags
+      3, 4, 5, 6, 7, 8, 9, 10, // Inner exception record address
+      8, 7, 6, 5, 4, 3, 2, 1,  // Exception address
+      4, 0, 0, 0, 0, 0, 0, 0,  // Parameter count, padding
+      0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, // Parameter 0
+      0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, // Parameter 1
+      0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, // Parameter 2
+      0x40, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, // Parameter 3
+      0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, // Parameter 4
+      0x60, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, // Parameter 5
+      0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77, // Parameter 6
+      0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, // Parameter 7
+      0x90, 0x91, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97, // Parameter 8
+      0xa0, 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, // Parameter 9
+      0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5, 0xb6, 0xb7, // Parameter 10
+      0xc0, 0xc1, 0xc2, 0xc3, 0xc4, 0xc5, 0xc6, 0xc7, // Parameter 11
+      0xd0, 0xd1, 0xd2, 0xd3, 0xd4, 0xd5, 0xd6, 0xd7, // Parameter 12
+      0xe0, 0xe1, 0xe2, 0xe3, 0xe4, 0xe5, 0xe6, 0xe7, // Parameter 13
+      0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, // Parameter 14
+      // Thread Context
+      0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, 0x88, // DataSize, RVA
+  };
+  auto ExpectedFile = create(Data);
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+  const MinidumpFile &File = **ExpectedFile;
+  Expected<const minidump::ExceptionStream &> ExpectedStream =
+      File.getExceptionStream();
+  ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
+  EXPECT_EQ(0x04030201u, ExpectedStream->ThreadId);
+  const minidump::Exception &Exception = ExpectedStream->ExceptionRecord;
+  EXPECT_EQ(0x02040302u, Exception.ExceptionCode);
+  EXPECT_EQ(0x09080807u, Exception.ExceptionFlags);
+  EXPECT_EQ(0x0a09080706050403u, Exception.ExceptionRecord);
+  EXPECT_EQ(0x0102030405060708u, Exception.ExceptionAddress);
+  EXPECT_EQ(4u, Exception.NumberParameters);
+  for (uint64_t index = 0; index < Exception.MaxParameters; ++index) {
+    EXPECT_EQ(0x1716151413121110u + index * 0x1010101010101010u,
+              Exception.ExceptionInformation[index]);
+  }
+  EXPECT_EQ(0x84838281, ExpectedStream->ThreadContext.DataSize);
+  EXPECT_EQ(0x88878685, ExpectedStream->ThreadContext.RVA);
+}