Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Poison address-exposed user variables in debug #54685

Merged
merged 12 commits into from
Jul 1, 2021
Merged

Poison address-exposed user variables in debug #54685

merged 12 commits into from
Jul 1, 2021

Conversation

jakobbotsch
Copy link
Member

Fix #13072

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 24, 2021
@jakobbotsch jakobbotsch changed the title Poison debug variables Poison address-exposed user variables in debug Jun 24, 2021
@jakobbotsch
Copy link
Member Author

@sandreenko I hope I have addressed what we talked about some weeks ago.

  1. Now we only poison address exposed user locals
  2. The poison code is emitted in a scratch BB right after the prolog
  3. It's implemented for ARM/ARM64 as well

PTAL

@sandreenko
Copy link
Contributor

cc @dotnet/jit-contrib

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Jun 24, 2021 •
edited
Loading

I want to run local tests first before I do some more testing in CI, not sure that this is quite ready yet.

sandreenko
sandreenko reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM, this is an elegant solution, a few nits/questions.

src/coreclr/jit/compiler.h Outdated Show resolved Hide resolved
src/coreclr/jit/flowgraph.cpp Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs Show resolved Hide resolved
src/coreclr/jit/codegencommon.cpp Show resolved Hide resolved
src/coreclr/jit/codegencommon.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegencommon.cpp Outdated Show resolved Hide resolved
@runfoapp runfoapp bot mentioned this pull request Jun 25, 2021
Closed
@jakobbotsch
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch jakobbotsch marked this pull request as ready for review June 28, 2021 23:09
@jakobbotsch
Copy link
Member Author

This should be ready for review. I believe the failing tests are #54778.
To make sure we do not allocate things to the scratch register being used for the immediate I have added a kill in register allocation when we see the scratch BB and know that we will emit poisoning. @kunalspathak can you take a look at this part?

cc @dotnet/jit-contrib

kunalspathak
kunalspathak reviewed Jun 29, 2021
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Could you also include a test for this?

src/coreclr/jit/flowgraph.cpp Outdated Show resolved Hide resolved
jakobbotsch and others added 3 commits June 29, 2021 20:25
@jakobbotsch
Add a test
3d9fd8c
@jakobbotsch @kunalspathak
Rephrase comment
8c56578 
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@jakobbotsch
Disable poisoning test on mono
302bfba
kunalspathak
kunalspathak approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

src/coreclr/jit/flowgraph.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member Author

jakobbotsch commented Jul 1, 2021 •
edited
Loading

I added a test. Failing tests are #54778 and some Mono failures (looks like https://github.com/dotnet/runtime/issues/53329#issuecomment-871039590). I'll wait for @echesakovMSFT review before merging.

sandreenko
sandreenko approved these changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM

echesakov
echesakov approved these changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

LGTM

@jakobbotsch jakobbotsch merged commit b8beed2 into dotnet:main Jul 1, 2021
@jakobbotsch jakobbotsch deleted the poison-debug-variables branch July 1, 2021 19:39
@sandreenko sandreenko mentioned this pull request Jul 2, 2021
Closed
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 2, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_hot_reload_debug
946c391 
* origin/main: (27 commits)
  [mono][llvm] Only emit 'LLVM failed' messages on verbosity > 0. (dotnet#55060)
  Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY (dotnet#54625)
  [main] Update dependencies from dnceng/internal/dotnet-optimization dotnet/arcade dotnet/xharness dotnet/hotreload-utils (dotnet#55007)
  disable a failing test. (dotnet#55063)
  [mono][wasm] Disable some tests which crash on AOT. (dotnet#55054)
  Fix fix_allocation_context for regions (dotnet#54931)
  Delete stale references to System.IO.FileSystem.Primitives (dotnet#55041)
  Add binplaced analyzers to ASP.NET transport package (dotnet#55042)
  [mono] Enable many HardwareIntrinsic tests on wasm
  Delete `compQuirkForPPP`. (dotnet#55050)
  [Mono] Condition Workload AOT import to be osx only (dotnet#55040)
  package native quic library (dotnet#54992)
  Make GlobalizationMode code consistent (dotnet#55039)
  Expand PerfMap format to support metadata for symbol indexation (dotnet#53792)
  [debugger]Componentize debugger (dotnet#54887)
  [Mono] Include loaded interpreter methods as EventPipe session rundown method events. (dotnet#54953)
  Delete stale ActiveIssue from HttpHeadersTest (dotnet#55027)
  Poison address-exposed user variables in debug (dotnet#54685)
  Recategorize emsdk dependency (dotnet#55028)
  Remove the the wasm AOT specific test project exclusions (dotnet#54988)
  ...
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@echesakov echesakov echesakov approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

@sandreenko sandreenko sandreenko approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fill uninitialized address-taken locals with poison pattern in debug codegen
4 participants
@jakobbotsch @sandreenko @echesakov @kunalspathak