Thanks all for testing this! I had no idea it was this hard to get a non-executable section :)

The Windows failure (https://reviews.llvm.org/D71372#1793371) seems like it may also be caused by the underscore symbol issue.

Thanks for looking into this @jankratochvil!

Took a quick look, and I think the good news is that the failure looks fairly superficial.

I believe all the comments should be addressed at this point; thanks very much for your reviews so far.

I wanted to also mention that this patch won't address buggy behavior if, for example, the stub takes a function pointer on the stack (vs a normal data pointer). In this case, the executable check will succeed, and the breakpoint will be written, but to a potentially arbitrary code address. This may or may not lead to the debugger "randomly" breaking out later in a debug session if or when that code happens to be executed (I've reproduced this, but also seen some very confusing behavior where in some situations the breakpoint logging will say a breakpoint is written, but it doesn't seem like this was actually the case. The presence of instructions that manipulate the stack pointer seemed to affect this also..)

I noticed some other tests specify the OS's they apply to-- I wasn't sure if there were any restrictions I should put on this one. I only tested on MacOS, and I imagine it should work on Linux. Not sure about Windows.

Add test.

In D71372#1782536, @clayborg wrote:

I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return

[0x00000000 - 0x100000000) ---

no permisssions are represented as "---". Then you can take the end address and look it up, and continue. So as long as we aren't getting bogus values back, we are good.

Screenshots with new error messages.

Output error messages to console in addition to logging.

So just add a std::string to ThreadPlanStepOut, and cons up your error message there. Then in ThreadPlanStepOut::ValidatePlan, instead of doing:

if (m_return_bp_id == LLDB_INVALID_BREAK_ID) {
  if (error)
    error->PutCString("Could not create return address breakpoint.");
  return false;
}

Write the error string you made in the constructor.

It's a good idea to log it as well, since the logs don't get command output so having it there will make logs easier to read.

In D71372#1781316, @labath wrote:

I'm not sure how easy it is to do that here, but it would certainly be nice to include these error messages in the actual error output from the "finish" command so that they are visible even without logging enabled.

Current output.

Output return address in logging

Remove lingering Section.h include

Something I noticed while updating the patch was that the GetLoadedAddressPermissions call would succeed, even if passed an address that is obviously not mapped. In my test case I placed an int 0x22 where the return address would be, expecting the validation to fail at the GetLoadAddressPermissions call because 0x22 is not mapped. However, it only ended up failing when the permissions (which were empty) were checked for an execute bit. It seems to me like this might be another bug, but I'm not sure.

Here are a few screenshots of what the bug looks like:

Updated to get permissions information from the Process rather than the Section.