This is an archive of the discontinued LLVM Phabricator instance.

Don't fail step out if remote server doesn't implement qMemoryRegionInfo
ClosedPublic

Authored by ted on Jan 10 2020, 8:23 AM.

Details

Summary

The return address validation in D71372 will fail if the memory permissions can't be determined. Many embedded stubs either don't implement the qMemoryRegionInfo packet, or don't have memory permissions at all.

Remove the return from the if clause that calls GetLoadAddressPermissions, so this call failing doesn't cause the step out to abort. Instead, assume that the memory permission check doesn't apply to this type of target.

Diff Detail

Event Timeline

ted created this revision.Jan 10 2020, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 8:23 AM
labath accepted this revision.Jan 13 2020, 2:50 AM

This seems fine to me, though I do get a feeling that without the testcase, some refactor will break this sooner or later. Making a test case for this would be tricky (though not impossible) and this it wasn't even your patch that broke this, so I don't think we should vehemently require a test to go with this. However, creating something might still be worthwhile, since you're the one who's going to have to debug any breakages...

This revision is now accepted and ready to land.Jan 13 2020, 2:50 AM
jingham requested changes to this revision.EditedJan 28 2020, 11:45 AM

I don't think you want to put anything in m_constructor_errors in this case either. It's fine to log something out, but ValidatePlan uses m_constructor_errors to help report a problem. If the breakpoint failed to take for some other reason, then we would report a permissions problem which is not the reason for the failure.

This revision now requires changes to proceed.Jan 28 2020, 11:45 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Jan 28 2020, 11:46 AM
This revision was automatically updated to reflect the committed changes.
ted added a comment.Jan 28 2020, 1:02 PM

Wow @jingham you posted your comment and I landed the patch at the same time!

I'll remove the m_constructor_errors.Printf line as requested.

ted updated this revision to Diff 240985.Jan 28 2020, 1:54 PM

Removed the code that sets m_constructor_errors when GetLoadAddressPermissions returns False, as requested by @jingham .

ted reopened this revision.Jan 28 2020, 1:55 PM
ted added a comment.Feb 5 2020, 1:09 PM

@jingham friendly ping

jingham accepted this revision.Feb 5 2020, 2:02 PM

Oops, sorry. Looks fine.

This revision is now accepted and ready to land.Feb 5 2020, 2:02 PM
This revision was automatically updated to reflect the committed changes.