[Lldb-commits] [PATCH] Implemented "thread jump" built-in command.

[jingham at apple.com]

On Aug 26, 2013, at 5:10 PM, Richard Mitton <richard at codersnotes.com> wrote:

> Hi Jim,
> 
> Thanks for the comments, I'll revise the patch to include some of the suggestions and resubmit.
> 
> > 1) Why do you allow multiple files to be specified on input?
> 
> This was not the intention, but as you mentioned the command parser API does not seem to support this as it stands right now. I'll add some manual checks, but I think this is ultimately something the option parser should be taking care of (as well as things like parsing integers etc)
> 
> > 3) I'm not sure that doing a generic file & line number search using the AddressResolverFileLine is the most efficient way to do what you want. That is going to search all the line tables in all the modules loaded in the program, which could be quite a large amount of data. Plus, you know in advance that you are only interested in the specified file & line in the current frame's function.
> 
> I don't know for sure that the user is jumping to the same function (they could jump elsewhere, which may or may not make sense for the current situation, but that's their decision). If they're debugging hand-written assembly instead of C, it can be totally valid.

In gdb, if you used the "jump" command to go outside the current function, you'd just get an error.  The bad effects of inadvertently jumping to another function make wise to protect against it.  Even in assembly you'd have to be very careful how you did this, since the stack teardown could be different between the two functions.  I'd vote for continuing to make it an error.  After all, you wouldn't want a simple line number mistype to scotch a user's debugging run for them.  And if somebody really wants to do this, they can always set the PC to the other function directly using register write pc.  

At the very least you should have a --force flag that you have to set before we will to jump outside the current function.  But if I were doing it, I would just make this an error.

And if you're going to continue with the broad net search, you should still limit your address search to the current function's module.  Even if you are going to allow cross-function jumps, cross shared library jumps seem really ill-advised.

> 
> > 4) This is minor, but the way you have written it, if there are multiple instances of the file/line in the current function, you go to the first one without any warning. I don't know if there's any better choice for where to go, but maybe you want to at least warn in that case.
> 
> For debug builds, there will probably be only one match. The situation I was thinking of is in optimized builds, where the source line could basically end up anywhere. In this case, the line-jump operation itself becomes kind of meaningless, so the best we can do is to jump where we think it should go, letting the user see the change in the PC themselves and decide whether it was valid. I'm not sure if it's even right to warn about this, I think it's just something you accept when trying to debug optimized code.

Dunno, it might be nice to see:

Line foo.c:12 had three address entries:
0xAAAA
0xAAAB
0xAAAC

choosing 0xAAAA

Then you could look at the options (since you've gone to the trouble of digging them up) and decide which one was more suitable.

> 
> > 5) This is extra credit, but we try not to have useful operations that can be done with the command line but not with the SB API's or vice versa.
> 
> This makes sense, I'll move the core code into a Frame::JumpPC function and make the target command call that. (and add an API wrapper too)

Cool, thanks for working on this.

Jim


> 
> Thanks for reviewing this,
> 
> Richard Mitton
> richard at codersnotes.com
> 
> On 08/26/2013 04:32 PM, jingham at apple.com wrote:
>> Richard,
>> 
>> Thanks for the patch.  I have a few comments.
>> 
>> 1) Why do you allow multiple files to be specified on input?  You don't seem to do anything with anything but the first file, so this just adds complexity to the code, and makes the workings of the command less clear.  The error will also be weird if you supplied two files, but
>> 
>> Note, one of the things that is currently missing from the Command Interpreter is a way to specify whether multiple entries of some option are allowed or not.  So for now you have to handle this by hand, by checking whether your m_file ivar is empty on assign and erroring out of the parsing if not.
>> 
>> 2) I try not to write commands that don't report parse errors in input immediately.  So for instance, I would do:
>> 
>>                 case 'l':
>>                     bool success;
>> 		    size_t tmp_line_num = Args::StringToUInt32( option_arg, 0, 0, &success);
>>                     if (!success)
>> 			error.SetErrorStringWithFormat("invalid line number: \"%s\"", option_arg);
>>                     else
>>                         m_line_num = tmp_line_num;
>>                     break;
>> 
>> that way you won't end up getting into funny situations when some option defaults back to 0 because the input was wrong.
>> 
>> 3) I'm not sure that doing a generic file & line number search using the AddressResolverFileLine is the most efficient way to do what you want.  That is going to search all the line tables in all the modules loaded in the program, which could be quite a large amount of data.  Plus, you know in advance that you are only interested in the specified file & line in the current frame's function.  It might be much easier to run through the line table for the current function for a file & line match.
>> 
>> There isn't a pre-packaged way to get the beginning and ending line table entries for a given function, but it is pretty easy to do.  You have the current function already, so get the CompileUnit from that, get its line table, and use LineTable::FindLineEntryByAddress - passing in the base address of the function's AddressRange - to get the entry & index for the start line table entry.  Then iterate through the line table using the index till the address is past the end address of the function.
>> 
>> Anyway, if you want to keep with your current scheme, you can certainly make a SearchFilter for the module containing the current frame's function, that will narrow down the search considerably.
>> 
>> 4) This is minor, but the way you have written it, if there are multiple instances of the file/line in the current function, you go to the first one without any warning.  I don't know if there's any better choice for where to go, but maybe you want to at least warn in that case.
>> 
>> 5) This is extra credit, but we try not to have useful operations that can be done with the command line but not with the SB API's or vice versa.  Seems like it would be pretty natural to add an SBFrame::JumpPC alongside SBFrame::SetPC.
>> 
>> Again, thanks for working on this.
>> 
>> Jim
>> 
>> On Aug 23, 2013, at 12:34 PM, Richard Mitton <richard at codersnotes.com> wrote:
>> 
>>>  <ping/>
>>> 
>>>  So, I just wanted to check here; the LLVM developer policy says I'm
>>>  expected to get every patch reviewed before submission. In general, for
>>>  lack of any feedback, is it OK to just submit stuff like this without
>>>  approval?
>>> 
>>>  Richard Mitton
>>>  richard at codersnotes.com
>>> 
>>> http://llvm-reviews.chandlerc.com/D1452
>>> _______________________________________________
>>> lldb-commits mailing list
>>> lldb-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>