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

[jingham at apple.com]

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