Page MenuHomePhabricator

Add ExceptionStream to llvm::Object::minidump
ClosedPublic

Authored by JosephTremoulet on Oct 8 2019, 11:50 AM.

Details

Summary

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

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 11:50 AM
labath added a comment.Oct 9 2019, 9:08 AM

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?

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.

  • Remove the os-defined exception code enum values

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...

  • Remove ExceptionCode enumeration

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.

LGTM, Pavel?

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?

  • Remove useless comment and leftover debugging cruft
JosephTremoulet marked 3 inline comments as done.Oct 11 2019, 7:54 AM
JosephTremoulet added inline comments.
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.

labath accepted this revision.Oct 11 2019, 8:12 AM
This revision is now accepted and ready to land.Oct 11 2019, 8:12 AM
This revision was automatically updated to reflect the committed changes.
JosephTremoulet marked an inline comment as done.