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

[Richard Mitton]

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.

 > 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.

 > 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)

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