Page MenuHomePhabricator

lldb more verbose "invalid frame" error
Needs ReviewPublic

Authored by pawelo on Sep 16 2014, 10:05 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Many times during my recent activities I saw invalid frame error message. I found it totally useless - it does not give any clue why the error happened. It turned out that it can occur due to obvious reasons: no target selected, no process launched etc. With this patch it is easier to guess root cause of this error and save hours of unnecessary investigation.

Diff Detail

Event Timeline

pawelo updated this revision to Diff 13769.Sep 16 2014, 10:05 PM
pawelo retitled this revision from to lldb more verbose "invalid frame" error.
pawelo updated this object.
pawelo edited the test plan for this revision. (Show Details)
pawelo set the repository for this revision to rL LLVM.
pawelo added a subscriber: Unknown Object (MLST).

This looks like a good change, thank you for coming up with it. Could you put the statements on separate lines from the conditional expressions? Instead of

if (!m_exe_ctx.HasTargetScope()) result.AppendError (GetInvalidTargetDescription());

more like this -

if (!m_exe_ctx.HasTargetScope())

result.AppendError (GetInvalidTargetDescription());

or

if (!m_exe_ctx.HasTargetScope())
{

result.AppendError (GetInvalidTargetDescription());

}

(in this case I think not using the braces will look cleaner). I know we have a few examples of the if (cond) stmt; in lldb already but they are something we need to clean up at some point.

J

pawelo updated this revision to Diff 13820.Sep 17 2014, 11:23 PM
pawelo set the repository for this revision to rL LLVM.

changed as requested

tfiala added a subscriber: tfiala.Sep 18 2014, 9:09 AM

Jason, do you want to submit this or do you want me to run it through over
here?

emaste added a subscriber: emaste.Sep 18 2014, 10:07 AM

I applied the patch and was going to commit it, then I realized I don't clearly understand what behavior we're fixing. e.g. these commands all report a reasonable error msg when a thread/process/frame is missing before the patch is applied.

% lldb
(lldb) f
error: invalid thread
(lldb) bt
error: invalid process
(lldb) reg read pc
error: invalid frame
(lldb) c
error: invalid process
(lldb)

pawelo updated this revision to Diff 13888.Sep 19 2014, 1:40 PM
pawelo set the repository for this revision to rL LLVM.

Maybe this looks better?

Looks good to me! Anyone else?

This looks good to me.

Yep good here too.