Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix step with stackalloc #27246

Merged
merged 5 commits into from
Oct 18, 2019
Merged

Fix step with stackalloc #27246

merged 5 commits into from
Oct 18, 2019

Conversation

sdmaclea
Copy link

@sdmaclea sdmaclea commented Oct 16, 2019

Generalize handling of stackalloc. Funclets were just a special case of an in function stack allocation.

Currently includes 5 commits

  1. Non functional change to make mReturnFrame private. This prepares for further commits.
  2. Add ability to get the return frame for the root managed method. This is not a functional change (yet)
  3. Revise IsRangeAppropriate to use the return frames SP to determine if we are in the same frame.
  4. Typos
  5. Fix a bug in the argument handling in the GetFunctionFromToken() API. This fixes a hang which blocked testing. Issue also occurred in the baseline.

Fixes #14926
This is a safer alternative to #26688

/cc @dotnet/dotnet-diag

@sdmaclea sdmaclea added this to the 5.0 milestone Oct 16, 2019
@sdmaclea sdmaclea requested review from noahfalk and hoyosjs October 16, 2019 23:41
@sdmaclea sdmaclea self-assigned this Oct 16, 2019
src/debug/ee/controller.cpp Outdated Show resolved Hide resolved
In case the active frame has no managed caller, capture
the unmanaged frame
Generalize handling of stack allocations and stepping
Check token type is a method before creating a CordbFunction.

Add extra assert to check for invalid tokens
@sdmaclea sdmaclea changed the title WIP Fix step with stackalloc Fix step with stackalloc Oct 17, 2019
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

In regards to 3.1, my guess is that shiproom won't take this unless you have evidence that this issue will be dramatically more prevalent+painful than it appears to have been in the past. My understanding of the stakes may not be accurate, but from what I see now I'd suggest not going to shiproom with it.

@@ -1550,7 +1550,8 @@ HRESULT CordbModule::GetFunctionFromToken(mdMethodDef token,
RSLockHolder lockHolder(GetProcess()->GetProcessLock());

// Check token is valid.
if ((token == mdMethodDefNil) ||
if ((token == mdMethodDefNil) ||
(TypeFromToken(token) != mdtMethodDef) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you hit this error even without the runtime portion of the change, or it only shows up together with the runtime change? If it is the former I think this is fine. If it is the latter I'd be worried that the runtime change has broken something we don't fully understand and a bad token here happened to be one symptom of it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This occurred in the baseline too. It could be a separate
PR.

@sdmaclea
Copy link
Author

In regards to 3.1, my guess is that shiproom won't take this unless you have evidence that this issue will be dramatically more prevalent+painful than it appears

FYI /cc @davidfowl

@sdmaclea sdmaclea merged commit 551710b into dotnet:master Oct 18, 2019
@sdmaclea sdmaclea deleted the stepStackalloc branch October 18, 2019 16:04
sdmaclea added a commit to sdmaclea/coreclr that referenced this pull request Oct 21, 2019
* Make ControllerStackInfo::m_returnFrame private

* Make ControllerStackInfo always capture a return frame

In case the active frame has no managed caller, capture
the unmanaged frame

* Fix step over stackalloc

Generalize handling of stack allocations and stepping

* Fix GetFunctionFromToken() argument checking

Check token type is a method before creating a CordbFunction.

Add extra assert to check for invalid tokens
sdmaclea added a commit that referenced this pull request Oct 22, 2019
* Make ControllerStackInfo::m_returnFrame private

* Make ControllerStackInfo always capture a return frame

In case the active frame has no managed caller, capture
the unmanaged frame

* Fix step over stackalloc

Generalize handling of stack allocations and stepping

* Fix GetFunctionFromToken() argument checking

Check token type is a method before creating a CordbFunction.

Add extra assert to check for invalid tokens
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugging a stackalloc line takes multiple F10s
3 participants