[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

Fri Jan 10 08:25:13 PST 2020

ted added a comment.

In D71372#1813703 <https://reviews.llvm.org/D71372#1813703>, @labath wrote:

> In D71372#1813142 <https://reviews.llvm.org/D71372#1813142>, @ted wrote:
>
> > In D71372#1811594 <https://reviews.llvm.org/D71372#1811594>, @labath wrote:
> >
> > > In D71372#1810687 <https://reviews.llvm.org/D71372#1810687>, @ted wrote:
> > >
> > > > I've got another failure case for this. If the remote gdbserver doesn't implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will fail.
> > > >  ....
> > >
> > >
> > > That's a good point Ted. I think we should give targets which don't support fetching permissions the benefit of the doubt, and treat all memory as potentially executable. Would removing the `return` statement from the `if(!GetLoadAddressPermissions)` branch solve your problem? If so, can you whip up a patch for that?
> >
> >
> > Removing the return statement fixes the issue. I'll put up a patch. Keeping the m_constructor_errors.Printf line doesn't cause a failure; it might be useful to keep that in case the breakpoint can't be created for other reasons. What do you think?
>
>
> I don't care much either way.. Since you have this kind of a target around, you can judge whether printing this error/warning after each "finish" would be useful or just annoying. Another possibility would be to don't print the error to the command output, but still emit something into the log...

There's no message printed after each finish; I think the message will only be printed if there's another error. That's fine by me.

I've uploaded the patch in D72513 <https://reviews.llvm.org/D72513>.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372