[Lldb-commits] [PATCH] D73303: [lldb/Target] Add Abort StackFrame Recognizer

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:81
+    ThreadSP thread_sp, FileSpec module_spec, ConstString function_name) {
+  const uint32_t frames_to_fetch = 10;
+  StackFrameSP prev_frame_sp = nullptr;
----------------
jingham wrote:
> mib wrote:
> > friss wrote:
> > > mib wrote:
> > > > JDevlieghere wrote:
> > > > > Magic value? Why 10?
> > > > Pretty much. In the beginning I was unwinding the entire stack. But if there is too much frames (i.e. recursion) this would take too much time. We agreed with @friss and @jingham to unwind up to 10 frames at most.
> > > > 
> > > > Can you think of a better way to do it ?
> > > I don't think that's exactly what I said. My opinion was that asserts will have a pretty deterministic layout, and that we should know exactly what to unwind to determine whether we have an assert or not.
> > > 
> > > In your test, you actually check that the frame with the assert is frame 4. If it is anything other then 4 in any configuration, then the test will fail. You might as well just put 4 in the code here rather than a magical 10.
> > The number of frames to unwind is different depending on the platform. I chose 10 to give us some extra margin but I'll change it to 4 or 5,
> We want this as small as will do the job - since we don't want to unwind more than necessary.  But we aren't going to do this unwinding on every stop - only when we actually see a thread with frame 0 in __pthread_kill (or whatever the system kill is).  So we don't need to over optimize.  It would be silly to go have to fix this because some platform added another layer between assert and kill.
> 
> I'd go see what it is on all the Unixes we know about and add a couple to future proof it.
> 
> The test should definitely not depend on how many frames there are between the assert and the _pthread_kill.  It should just check that the currently selected frame has the right file & line number.  If you want to keep this a shell test, you could backtrace the thread and make sure the * is on the line with "assert.c", or maybe try:
> 
> ```
> (lldb) source info
> Lines found in module `killme
> [0x0000000100000f2e-0x0000000100000f6f): /tmp/assert.c:7:3
> 
> ```
> 
> If the frame wasn't selected properly this will show:
> 
> 
> ```
> (lldb) source info
> error: No debug info for the selected frame.
> 
> ```
Actually, the test you have is fine if you just leave out the "4".  You really only care about the "assert.c".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73303