Cleanup after GC info version update. #108117

VSadov · 2024-09-22T23:24:10Z

There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.
Some code moved around to tease apart reachable and unreachable scenarios.

remove code specific to no longer used GC info versions
separate major parts that deal with X86 specific scenarios from everything else
once we have X86 and "everything else", we can make "everything else" much simpler.
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
look for more things, address TODOs

VSadov · 2024-09-23T00:14:31Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-09-23T00:14:43Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-09-23T19:49:26Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-09-23T19:49:38Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-09-24T00:26:13Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-09-24T00:26:26Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-09-24T02:13:00Z

I think this is ready for review.
Cc: @jkotas @janvorli @jakobbotsch

jkotas · 2024-09-24T02:25:24Z

src/coreclr/vm/gccover.cpp

 {
    _ASSERTE(!nativeCodeVersion.IsNull());

+    // CONSIDER: does anyone call this with fZapped == true ? are there plans?


Zap was a code name for fragile NGen.

Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped path as a part this change.
Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.

I will check what happens with R2R.

VSadov · 2024-09-24T06:59:14Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-09-24T06:59:36Z

Azure Pipelines successfully started running 2 pipeline(s).

janvorli · 2024-09-24T11:47:03Z

@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.

VSadov · 2024-09-24T19:12:16Z

@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.

Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.
I'll see if I could rearrange the X86 part a bit so diffs are smaller.

VSadov · 2024-09-24T21:39:10Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-09-24T21:39:22Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-09-24T21:55:00Z

src/coreclr/vm/gccover.cpp

+// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
+//
+// This race is now mitigated below. Where we won't initiate a stress mode GC
+// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.


The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.

Corrected the explanation of how the race may happen and brought the mitigation back.

VSadov · 2024-09-25T18:15:55Z

Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?

It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.

I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)

VSadov · 2024-09-25T20:30:43Z

I'll see if I could rearrange the X86 part a bit so diffs are smaller.

@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.

I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.

The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.

I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.

I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.
That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.

The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.
(and I did verify in debugger that we actually stress things :-)

VSadov · 2024-10-15T18:18:16Z

Rebased onto recent main.

VSadov · 2024-10-15T18:19:17Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-10-15T18:19:29Z

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines · 2024-10-28T18:10:00Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-10-28T21:23:42Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-10-28T21:23:57Z

Azure Pipelines successfully started running 2 pipeline(s).

VSadov · 2024-10-30T20:08:18Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-10-30T20:08:38Z

Azure Pipelines successfully started running 2 pipeline(s).

…gThreads

VSadov · 2024-11-01T01:24:37Z

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-11-01T01:24:52Z

Azure Pipelines successfully started running 2 pipeline(s).

janvorli

LGTM, thank you!

VSadov · 2024-11-05T19:06:46Z

Thanks!!

VSadov added the area-VM-coreclr label Sep 22, 2024

dotnet-policy-service bot assigned VSadov Sep 22, 2024

This was referenced Sep 23, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Closed

Test failure: Assertion failed 'likelihood <= 1.0' #108100

Closed

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

VSadov marked this pull request as ready for review September 24, 2024 01:19

VSadov requested a review from MichalStrehovsky as a code owner September 24, 2024 01:19

jkotas reviewed Sep 24, 2024

View reviewed changes

VSadov force-pushed the gcVerCleanup branch from 9f4ef71 to eccfbae Compare September 24, 2024 21:38

VSadov commented Sep 24, 2024

View reviewed changes

VSadov mentioned this pull request Sep 25, 2024

Implement GC stress for R2R code. #108249

Open

VSadov force-pushed the gcVerCleanup branch from eccfbae to ff1d146 Compare October 15, 2024 18:17

VSadov force-pushed the gcVerCleanup branch from 686063b to 7875173 Compare October 28, 2024 21:22

VSadov added 16 commits October 31, 2024 18:24

ReadyToRunVersionToGcInfoVersion

99bd8a8

removed AreSafePointsInterruptible, IsInterruptibleSafePoint, etc..

890a89b

dead code

dab93df

cleaned non-x86 path

cc5d7d0

some dead code

26d0616

require disasm for fully interruptible x64

c617259

x86-specific cleanup

aed485c

not supporting multireg returns on x86

badda2a

some comments

9512ded

fix for ARM32

6560f4c

asserts

87382e5

another unreachable condition.

d3af824

remove unused findNextFunclet

ae5ef82

fix for UseRedirectForGcStress case.

804b0e5

update comments and revert mitigation for active ICF + g_TrapReturnin…

d866c53

…gThreads

fix after rebasing

f7dbe6a

VSadov force-pushed the gcVerCleanup branch from 3770e11 to f7dbe6a Compare November 1, 2024 01:24

janvorli approved these changes Nov 5, 2024

View reviewed changes

VSadov merged commit 552782f into dotnet:main Nov 5, 2024
122 checks passed

VSadov deleted the gcVerCleanup branch November 5, 2024 19:06

github-actions bot locked and limited conversation to collaborators Dec 6, 2024

Cleanup after GC info version update. #108117

Cleanup after GC info version update. #108117

Uh oh!

Conversation

VSadov commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Sep 23, 2024

Uh oh!

azure-pipelines bot commented Sep 23, 2024

Uh oh!

VSadov commented Sep 23, 2024

Uh oh!

azure-pipelines bot commented Sep 23, 2024

Uh oh!

VSadov commented Sep 24, 2024

Uh oh!

azure-pipelines bot commented Sep 24, 2024

Uh oh!

VSadov commented Sep 24, 2024

Uh oh!

jkotas Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

VSadov Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

VSadov commented Sep 24, 2024

Uh oh!

azure-pipelines bot commented Sep 24, 2024

Uh oh!

janvorli commented Sep 24, 2024

Uh oh!

VSadov commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Sep 24, 2024

Uh oh!

azure-pipelines bot commented Sep 24, 2024

Uh oh!

VSadov Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

VSadov commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Oct 15, 2024

Uh oh!

VSadov commented Oct 15, 2024

Uh oh!

azure-pipelines bot commented Oct 15, 2024

Uh oh!

azure-pipelines bot commented Oct 28, 2024

Uh oh!

VSadov commented Oct 28, 2024

Uh oh!

azure-pipelines bot commented Oct 28, 2024

Uh oh!

VSadov commented Oct 30, 2024

Uh oh!

azure-pipelines bot commented Oct 30, 2024

Uh oh!

VSadov commented Nov 1, 2024

Uh oh!

azure-pipelines bot commented Nov 1, 2024

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Nov 5, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

VSadov commented Sep 22, 2024 •

edited

Loading

VSadov commented Sep 24, 2024 •

edited

Loading

VSadov commented Sep 25, 2024 •

edited

Loading

VSadov commented Sep 25, 2024 •

edited

Loading