Improve SBThread's stepping API using SBError parameter.
Needs ReviewPublic

Authored by apolyakov on Sun, Jun 10, 7:50 AM.

Details

Summary

The new methods will allow to get error messages from stepping methods.

Diff Detail

apolyakov created this revision.Sun, Jun 10, 7:50 AM

Also, I suggest to make similar things in other SBThread methods like StepOut etc.

This looks reasonable to me, but I'd like to also hear from Greg and Jim since SBAPI changes are kind of final.

include/lldb/API/SBThread.h
96

Where is the SBAPI documentation that is displayed on http://lldb.llvm.org ? Apparently no in this file, but it must exist *somewhere*, can you update it too, please?

clayborg accepted this revision.Mon, Jun 11, 8:37 AM

This is fine. Make sure all other steps have error overloads.

This revision is now accepted and ready to land.Mon, Jun 11, 8:37 AM

It is unfortunate that we can't overload with the return value and return an SBError...

apolyakov added inline comments.Mon, Jun 11, 9:31 AM
include/lldb/API/SBThread.h
96

Due to http://lldb.llvm.org/: This documentation is generated directly from the source code with doxygen.. So I think that we don't need to change anything except source files.

I found it out. Info about each method is located in header file. For example, you may look at include/lldb/API/SBThread.h and find text from docs.

Hmm.. it looks like most C++ API calls don't have any documentation.

http://lldb.llvm.org/cpp_reference/html/classlldb_1_1SBThread.html#a42755a170e127881a5dd65162217f68b

It does look like the python API has more documentation.. where does that come from?

scripts/interface/SBThread.i

It would be good to add documentation for the new API call there, then.

Hmm.. it looks like most C++ API calls don't have any documentation.

http://lldb.llvm.org/cpp_reference/html/classlldb_1_1SBThread.html#a42755a170e127881a5dd65162217f68b

It does look like the python API has more documentation.. where does that come from?

scripts/interface/SBThread.i

Do you know what is the difference between autodoc and docstring there?

jingham requested changes to this revision.Mon, Jun 11, 10:27 AM

Sorry for coming in late (WWDC...) It does seem asymmetric to only have one stepping API take an error. If one is going to they all should.

You need to add the new API to the SBThread.i file as well or it won't get exported to Python.

Beyond that, the SBError parameter is the last parameter in pretty much all the other SB API's. We do inputs first and outputs second as a general rule. So if you are going to add one here it would be better to make it the last parameters.

BTW, the web docs are auto-generated but not live. I don't remember when somebody last refreshed them (and actually I didn't follow in detail how this was done.) But we should kick that as an additional step.

This revision now requires changes to proceed.Mon, Jun 11, 10:27 AM
lemo added a subscriber: lemo.Mon, Jun 11, 10:30 AM
lemo added inline comments.
include/lldb/API/SBThread.h
96

While not perfectly consistent across SB API, most methods having an out SBError parameter place it last.

96

I believe you need to update SBThread.i as well. It is required for Python binding through SWIG and it also defines the Python API documentation.

apolyakov retitled this revision from Add method SBThread::StepOver with SBError parameter. to Improve SBThread's stepping API using SBError parameter..
apolyakov edited the summary of this revision. (Show Details)

Documented changes and overloaded all stepping functions.

Have you looked into making the error the first vs the last argument? If the majority of all SBAPI calls put the error last, we should do this here, too.

Have you looked into making the error the first vs the last argument? If the majority of all SBAPI calls put the error last, we should do this here, too.

As you could see, SBError is on first place only in one function - StepOver. When I replaced it to last place, I got an error:

/home/alexander/workspace/gsoc/llvm/tools/lldb/scripts/./interface/SBThread.i:218: Error: Non-optional argument 'error' follows an optional argument.

so I returned it back.

By the way, we can do something like:

void StepOver() {
  StepOver(lldb:eOnlyDuringStepping);
}

void StepOver(lldb::RunMode stop_other_threads) {
  SBError error;
  StepOver(stop_other_threads, error);
}

and so on.

aprantl accepted this revision.Mon, Jun 11, 5:51 PM

Ah I see. That's because the last argument is a C++ default argument. It looks like the convention in this file is that the error argument should be the last non-defaulted argument.

Ah I see. That's because the last argument is a C++ default argument. It
looks like the convention in this file is that the error argument should be
the last non-defaulted argument.

I think it should be:

void StepOver(lldb::RunMode stop_other_threads, lldb::SBError &error); //
no default argument!

Ie. if you want the overload with error you need to pass RunMode
explicitly. Keeping the overload set manageable is a good practice in
general, not just because of SWIG (default arguments + overloaded arguments
can easily get out of hand)

apolyakov updated this revision to Diff 150913.Tue, Jun 12, 3:06 AM

Some readability improvements(more comments). I think that we should keep StepOver signature as

void StepOver(SBError &error, lldb::RunMode stop_other_threads);

since it seems that in SBThread methods SBError paramater is the last non-optional one, as @aprantl said.

aprantl added a comment.EditedTue, Jun 12, 9:08 AM
This comment has been deleted.
scripts/interface/SBThread.i
256

a -> an

source/API/SBThread.cpp
703–704

Please always prefer early exits (https://www.llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)

if (!exe_ctx.HasThreadScope())
  return error.SetErrorString("SBThread object is invalid");
754–755

same here

1168

and here.

Added early exits.

apolyakov added inline comments.Tue, Jun 12, 11:15 AM
source/API/SBThread.cpp
1138

@aprantl do you think that we should use early exit here? There is printing to log at the end of the function, so in case of early exit, we must duplicate log->Printf(). Is it worth it?

I agree with Leonard, for the StepOver overload that returns errors, just make the mode parameter required. That will reduce confusion.

I agree with Leonard, for the StepOver overload that returns errors, just make the mode parameter required. That will reduce confusion.

Works for me, too.

source/API/SBThread.cpp
1138

Duplicating the log message is probably not worth it.

1143–1144

It looks like the error string should be set here as well?

1149–1150

I think clang-format would do this as:

} else
   error.SetErrorString("this SBThread object is invalid");
1176

same here.

1185

Looking at the implementation of SBThread::Resume and SBThread::Suspend, the only difference seems to be the API called and the log message. If there is a third command with a similar implementation, it might be a fun exercise to factor that out into a helper function that takes std::function<> and a log string argument...

Added StepOver overload without optional parameters.

apolyakov added inline comments.Tue, Jun 12, 12:42 PM
source/API/SBThread.cpp
816

I found this place and added return.

Minor update: moved error.SetErrorString to the top of if statement to increase readability.

Looks good. Just a question about including the commented out default arguments

source/API/SBThread.cpp
636

might avoid showing the default argument in case it changes and we don't do this anywhere else that I know of.

678

Ditto

684

Ditto

691

Ditto

Looks good. Just a question about including the commented out default arguments

Don't you think it increases readability of this code?

Looks good. Just a question about including the commented out default arguments

Don't you think it increases readability of this code?

It does if it stays in sync. This isn't done anywhere else and I don't believe it is in the LLVM coding conventions. If it is, please correct me. I didn't mark this as Request Changes as I wanted to gauge what others think. I am happy to go either way, but if we do go this way, it will have impact on all patches going forward so I wanted to point it out for discussion.