[Lldb-commits] [PATCH] D68657: Update MinidumpYAML to use minidump::Exception for exception stream

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks. I think this is great. @grimar, do you have any more comments?



================
Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:377-383
+  // TODO: We could provide a reasonable default for ThreadContext by searching
+  // the Thread stream for a thread with the given ID and using its Context.
+  // That would require a couple changes:
+  //   1. We'd need to pass the whole dump around as a context argument.
+  //   2. We'd need to ensure that the Thread stream got processed before
+  //      the Exception stream (or make Exception's ThreadContext required
+  //      when the Exception stream is processed before the Thread stream).
----------------
JosephTremoulet wrote:
> labath wrote:
> > I've been thinking about this for a while now, and while that idea has some appeal, I am not sure if this would ever really be a "good" idea. Currently, this format allows you to easily create syntactically-valid-but-probably-nonsensical minidumps (multiple thread list streams, multiple threads with the same ID, etc..). All of that would be more difficult if we started depending on strict "correctness" of other parts of the minidump in order to compute something here.
> > 
> > Even if I was doing this, I'd probably implement this differently -- make the context always optional, but then check for consistency higher up (the validate call of the entire minidump object or something). Anyway, maybe just delete this todo? :)
> I've removed the TODO, but I reserve the right to think this would be a good idea :). Speficially because I think you could still model in YAML anything you could put in a minidump file, by explicitly providing the ThreadContext even when it has a default.
> 
> (This is in contrast to the other validation stuff I had in earlier revisions, where I was just misunderstanding the point of yaml validation -- so thanks for explaining it!) 
Fair enough. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68657/new/

https://reviews.llvm.org/D68657