friss 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.
>
> ```
You say it is 4 or 5, so your test will fail in one of those configurations as
you have hardcoded the frame number there.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73303/new/
https://reviews.llvm.org/D73303
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
