Improve SBThread's stepping API using SBError parameter.
ClosedPublic

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

Diff Detail

Repository
rL LLVM
apolyakov created this revision.Jun 10 2018, 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 ↗(On Diff #150650)

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.Jun 11 2018, 8:37 AM

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

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

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

apolyakov added inline comments.Jun 11 2018, 9:31 AM
include/lldb/API/SBThread.h
96 ↗(On Diff #150650)

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.Jun 11 2018, 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.Jun 11 2018, 10:27 AM
lemo added a subscriber: lemo.Jun 11 2018, 10:30 AM
lemo added inline comments.
include/lldb/API/SBThread.h
96 ↗(On Diff #150650)

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

96 ↗(On Diff #150650)

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.Jun 11 2018, 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.Jun 12 2018, 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.EditedJun 12 2018, 9:08 AM
This comment has been deleted.
scripts/interface/SBThread.i
257 ↗(On Diff #150913)

a -> an

source/API/SBThread.cpp
703 ↗(On Diff #150913)

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");
752 ↗(On Diff #150913)

same here

1148 ↗(On Diff #150913)

and here.

Added early exits.

apolyakov added inline comments.Jun 12 2018, 11:15 AM
source/API/SBThread.cpp
1136 ↗(On Diff #150983)

@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
1136 ↗(On Diff #150983)

Duplicating the log message is probably not worth it.

1141 ↗(On Diff #150983)

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

1146 ↗(On Diff #150983)

I think clang-format would do this as:

} else
   error.SetErrorString("this SBThread object is invalid");
1171 ↗(On Diff #150983)

same here.

1179 ↗(On Diff #150983)

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.Jun 12 2018, 12:42 PM
source/API/SBThread.cpp
816 ↗(On Diff #150996)

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 ↗(On Diff #151000)

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

678 ↗(On Diff #151000)

Ditto

684 ↗(On Diff #151000)

Ditto

691 ↗(On Diff #151000)

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.

apolyakov edited the summary of this revision. (Show Details)

I removed comments about optional arguments since I didn't find any info about it in LLVM coding style.

Just reminder that it still needs a review. Thanks in advance.

This patch is adding new overloads to SBAPI calls that don't return an SBError, such as:

// Old:
void StepOutOfFrame(SBFrame &frame);
// New:
void StepOutOfFrame(SBFrame &frame, SBError &error);

I wonder if it would be easier to use and more consistent with the rest of the API if we instead added an overload that returns an SBError, like this:

// New:
SBError StepOutOfFrameWithError(SBFrame &frame);
// Alternative names that are just as ugly.
SBError StepOutOfFrameE(SBFrame &frame);
SBError StepOutOfFrame2(SBFrame &frame);

@clayborg, @jingham: What do you think?

The SB API's tend to take SBError as an in/out parameter for the cases where the function in question naturally returns some other value, and return an SBError when there's no other logical return value.

So in these cases it would be more in line with the general practice to return an SBError. OTOH, I really don't like the practice of adding "Ex" or other similar function name decorations to get around the fact that C++ doesn't overload on return values. IMO they make the API harder to remember. And especially since we really should use the error version, so you will end up with the primary function having this weird tag as part of the name...

In balance I think adding it as an argument is better, but this is one of those cases where I would leave it to the taste of the implementor...

Jim

I think we just can make old versions of API methods returning SBError in all cases. This way we'll deal with SBError and won't break down API calls.
For example:

// old
void StepOver() {
  ...
}

// new
SBError StepOver() {
  ...
  return sb_error;
}
aprantl accepted this revision.Jun 20 2018, 11:22 AM

Okay.. then let's go with this version.

Won't this break client code that was calling StepOver? We are pretty serious about maintaining binary compatibility with the SB API's.

Jim

Won't this break client code that was calling StepOver? We are pretty serious about maintaining binary compatibility with the SB API's.

Yeah, we can't replace existing function calls: The C++ name mangling could change, and the calling convention could change, too, depending on how big SBError is.

If some client uses StepOver like sbThread.StepOver(), then making StepOver return SBError won't break anything since in this case returned SBError value will just be ignored. Am I missing something?

The client won't be expecting a struct return, and will have generated code to take a void return. If SBError happens to be returned in registers, nothing bad will happen, but if it's returned on the stack, that would corrupt the caller's stack. Relying on the particular kind of struct return to keep out of trouble seems like a bad practice to me.

Jim

The client won't be expecting a struct return, and will have generated code to take a void return. If SBError happens to be returned in registers, nothing bad will happen, but if it's returned on the stack, that would corrupt the caller's stack. Relying on the particular kind of struct return to keep out of trouble seems like a bad practice to me.

Jim

Ok, then I suggest not to increase amount of methods in API doing the same stuff and keep SBError as in/out parameter.

jingham accepted this revision.Jun 20 2018, 12:07 PM

That seems fine to me.

This revision is now accepted and ready to land.Jun 20 2018, 12:07 PM
This revision was automatically updated to reflect the committed changes.

This patch is adding new overloads to SBAPI calls that don't return an SBError, such as:

// Old:
void StepOutOfFrame(SBFrame &frame);
// New:
void StepOutOfFrame(SBFrame &frame, SBError &error);

I wonder if it would be easier to use and more consistent with the rest of the API if we instead added an overload that returns an SBError, like this:

// New:
SBError StepOutOfFrameWithError(SBFrame &frame);
// Alternative names that are just as ugly.
SBError StepOutOfFrameE(SBFrame &frame);
SBError StepOutOfFrame2(SBFrame &frame);

@clayborg, @jingham: What do you think?

that is the right way to do it, but we can't overload on return type only. We will need the old version of the code to be in the API for compatibility. Overloading by return type will result is two symbols with the same name. If we change the API over to return an SBError, we will crash programs that linked against the old API as it now becomes returns something, possibly a struct return type, so it will crash older programs.

that is the right way to do it, but we can't overload on return type only. We will need the old version of the code to be in the API for compatibility. Overloading by return type will result is two symbols with the same name. If we change the API over to return an SBError, we will crash programs that linked against the old API as it now becomes returns something, possibly a struct return type, so it will crash older programs.

I get that. My question was whether it is better to add a new overload that adds an SBError& argument versus adding a new overload that returns and SBError *and* has a different name. In any case, Jim pointed out that we are using both variants in other places, so it doesn't really seem to matter.