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.

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

Finally cloning #8551

Merged
merged 1 commit into from Jan 11, 2017

Conversation

Projects
None yet
9 participants
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Dec 9, 2016

Preliminary implementation of empty finally removal and finally cloning. Passing local tests for both x86 and x64 so figured I'd run it through CI to see what else might crop up.

Still a few important TODOs and potentially a lot of code cleanup left, so setting NO MERGE.

@benaadams

This comment has been minimized.

Copy link
Collaborator

benaadams commented Dec 9, 2016

Remove empty Dispose methods in finally also; to make the finally empty?

Thinking foreach loops with concrete enumerators e.g. I'm fairly sure I've seen functions with only foreach loops not inline due to error handling?

i.e. calling an empty Dispose in a finally block

@mikedn

This comment has been minimized.

Copy link
Contributor

mikedn commented Dec 9, 2016

Remove empty Dispose methods in finalizers also; to make the finalizer empty?

That seems completely unrelated. And who writes a finalizer that calls an empty Dispose method anyway?

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Dec 9, 2016

The arm debug failure looks odd and the test does not contain try/finally, so will retry.

@dotnet-bot retest Linux ARM Emulator Cross Debug Build

@benaadams

This comment has been minimized.

Copy link
Collaborator

benaadams commented Dec 9, 2016

who writes a finalizer that calls an empty Dispose method anyway?

@mikedn sorry, meant finally...

So for example this function is not inlinable

public static void ValidateHeaderCharacters(StringValues headerValues)
{
    foreach (var value in headerValues)
    {
        ValidateHeaderCharacters(value);
    }
}

with reason
[FAILED: has exception handling] FrameHeaders:ValidateHeaderCharacters(struct)

Because post C# compile it is actually

var e = headerValues.GetEnumerator();
try
{
    while (e.MoveNext())
    {
        ValidateHeaderCharacters(e.Current);
    }
}
finally
{
    e.Dispose();
}

Though it may not help that the Dispose method is defined via interface? So the il for the finally is

    finally
    {
      IL_0021: ldloca.s     V_0
      IL_0023: constrained. [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues/Enumerator
      IL_0029: callvirt     instance void [mscorlib]System.IDisposable::Dispose()
      IL_002e: endfinally   
    } // end of finally

The full il for the function is


  .method public hidebysig static void 
    ValidateHeaderCharacters(
      valuetype [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues headerValues
    ) cil managed 
  {
    .maxstack 1
    .locals init (
      [0] valuetype [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues/Enumerator V_0
    )

    // [247 38 - 247 50]
    IL_0000: ldarga.s     headerValues
    IL_0002: call         instance valuetype [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues/Enumerator [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues::GetEnumerator()
    IL_0007: stloc.0      // V_0
    .try
    {

      IL_0008: br.s         IL_0016
      // start of loop, entry point: IL_0016
        IL_000a: ldloca.s     V_0
        IL_000c: call         instance string [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues/Enumerator::get_Current()
        IL_0011: call         void Microsoft.AspNetCore.Server.Kestrel.Internal.Http.FrameHeaders::ValidateHeaderCharacters(string)
        IL_0016: ldloca.s     V_0
        IL_0018: call         instance bool [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues/Enumerator::MoveNext()
        IL_001d: brtrue.s     IL_000a
      // end of loop
      IL_001f: leave.s      IL_002f
    } // end of .try
    finally
    {
      IL_0021: ldloca.s     V_0
      IL_0023: constrained. [Microsoft.Extensions.Primitives]Microsoft.Extensions.Primitives.StringValues/Enumerator
      IL_0029: callvirt     instance void [mscorlib]System.IDisposable::Dispose()
      IL_002e: endfinally   
    } // end of finally
    IL_002f: ret          

  } // end of method FrameHeaders::ValidateHeaderCharacters
@mikedn

This comment has been minimized.

Copy link
Contributor

mikedn commented Dec 9, 2016

Though it may not help that the Dispose method is defined via interface

It doesn't matter, it is called on a struct so it's not a true virtual call. The JIT inlines Dispose and ends up with an empty finally (which should presumably be removed by the changes in this PR, see "Initial cut at empty finally removed" commit.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Dec 9, 2016

The changes in this PR are not going to impact inlining. However on the perf test referred to in aspnet/KestrelHttpServer#1233 I would expect that empty finally removal would give a decent improvement, since the iteration count of the test loops is small.

To enable inlining in cases like this we would need to add general support for inlining methods with EH, and then rely on this PR to clean things up afterwards. I haven't scoped how much work it would be to inline methods with EH, but suspect it is a fairly large work item.

improved performance and optimization of the common case where the try
completes without an exception.

Finally cloning will typically increase code size, though often the

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 10, 2016

Member

Since the funclet version needs to remain to handle the exceptional case, code size will increase unless the funclet call sequence (typically mov/call/nop) is bigger than the finally itself.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Updated to remove the typically ...

invariants regarding EH are put in place, and before most
other optimization, but run them after inlining
(so empty finallys can be more readily identified) and after the
addtion of implicit try-finallys created by the jit. Empty finallys

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 10, 2016

Member

typo: addtion

@AndyAyersMS AndyAyersMS force-pushed the AndyAyersMS:FinallyCloning branch from d1b6c41 to 9561f9a Dec 10, 2016

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Dec 10, 2016

Pushed an update that is rebased and picks up the formatting fix for flowgraph.cpp.

@benaadams

This comment has been minimized.

Copy link
Collaborator

benaadams commented Dec 11, 2016

Raised a Roslyn issue for foreach dotnet/roslyn#15824


We will remove empty finallys first, then clone.

Neither optimization will run in debug or min opts modes.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

To be clear, I believe when you say "in debug mode" you mean "when generating debuggable code"

do this yet.

Block weight/count maintenance. The cloned finally should reflect the
combined weight/count of the retarged call finallys.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

typo: retarged => retargeted?

readily computed. Selective cloning can be based on profile
feedback or other similar mechanisms for choosing the profitable
cases.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

Do you propose having at least a minimal cost metric to prevent excessive code size bloat, or do you propose to just always clone where legal and where our implementation can handle it? You could certainly imagine easy-to-construct cases that would bloat static code size tremendously.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

I had a similar thought. There is code already in place to count up the number of statements in the finally. It is perhaps too early to ask for anything more detailed.

We could put a circuit-breaker in based on that value. Let me gather some data on the size distribution for finallys based on this metric and we can consider a cutoff based on that.

@@ -149,7 +149,9 @@ For x86, all handlers are generated within the method body, typically in lexical

JIT64 attempts to speed the normal control flow by 'inlining' a called finally along the 'normal' control flow (i.e., leaving a try body in a non-exceptional manner via C# fall-through). Because the VM semantics for non-rude Thread.Abort dictate that handlers will not be aborted, the JIT must mark these 'inlined' finally bodies. These show up as special entries at the end of the EH tables and are marked with `COR_ILEXCEPTION_CLAUSE_FINALLY | COR_ILEXCEPTION_CLAUSE_DUPLICATED`, and the try_start, try_end, and handler_start are all the same: the start of the cloned finally.

JIT32 and RyuJIT currently do not implement finally cloning.
There is now a prototype for cloned finallys in RyuJIT. It currently does not handle the thread abort case.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

Obviously, you'll want to be more verbose about the details here when the implementation gets finalized. E.g., that thread abort isn't handled since cloned finally clauses aren't reported.

@BruceForstall

This comment has been minimized.

Copy link
Member

BruceForstall commented Dec 12, 2016

The spec / design doc looks really good. You could mention that finally cloning benefits hot/cold splitting because it puts the cloned finally in the main function region where it can be put in the hot region -- I think currently all funclets get put in the cold region, and on top of that we only can mark a single point where the hot and cold regions can be split.

To handle thread abort, I was thinking the cloned finallys would need to get a new EH region table that would both prevent (some?) code motion into/out of the region, but also mark the boundaries to be reported to the VM. This would have to be plumbed all through the JIT, which might be painful.

/* Empty Finally removal could remove basic blocks. Check that the flowgraph data is up-to-date */
fgDebugCheckBBlist(false, false);
#endif // DEBUG

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

IMO, these checks should be within the phase, so the top-level code to call the phases is cleaner.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Sure, will move them.

@@ -16889,6 +16889,26 @@ void Compiler::fgMorph()
fgDebugCheckBBlist(false, false);
#endif // DEBUG

/* Remove Empty Finallys */
fgRemoveEmptyFinally();

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

The comments here above your call to the phases don't seem useful.

(so empty finallys can be more readily identified) and after the
addtion of implicit try-finallys created by the jit. Empty finallys
may arise later because of optimization, but this seems relatively
uncommon.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

Seems like it would be pretty trivial to do a one-off measurement of how many empty finallys appear due to later optimization across a set of test assemblies, esp. using SuperPMI.

if (firstBlock != lastBlock)
{
JITDUMP("EH region %u finally has multiple basic blocks; skipping.\n", XTnum);
XTnum++;

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

Please use the format string:

EH#%u

when dumping EH region numbers, to make it easier to consistently be able to search for them in a JitDump.

//
// We don't expect to see retless finallys here, since
// the finally is empty.
noway_assert(currentBlock->isBBCallAlwaysPair());

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

Somewhere in here you're going to need some code to handle ARM and the finally target bit, e.g.:

#if FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)
            fgClearFinallyTargetBit(leaveBlk->bbJumpDest);
#endif // FEATURE_EH_FUNCLETS && defined(_TARGET_ARM_)

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Sure... We don't have pred lists yet so the bulk of that method won't do anything.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Fixed in the latest updates.

unsigned exampleEnclosedHandlerRegion = 0;

for (unsigned i = 0; i < compHndBBtabCount; i++)
{

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

you can bound the iteration by XTnum, since more nested regions precede less nested regions in the table (especially after EH canonicalization, which happens very early).

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Will do.

}

hasFinallyRet |= (block->bbJumpKind == BBJ_EHFINALLYRET);
isAllRare &= block->isRunRarely();

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

It always seems weird to me to use bitwise OR with 'bool' instead of something like:

hasFinallyRet = hasFinallyRet || (block->bbJumpKind == BBJ_EHFINALLYRET);

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

similar for bitwise AND, below


In reply to: 92069324 [](ancestors = 92069324)

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Using bool with |= or &= doesn't seem odd to me, but I'm fine changing it over.

//
// We assume the lexically first callfinally is the one that
// corresponds to the normal try exit point.
BasicBlock* normalCallFinallyBlock = nullptr;

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 12, 2016

Member

Can you justify this assumption?

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 13, 2016

Member

intuitively I would expect the last, not the first, to be the lexical normal exit case.


In reply to: 92069803 [](ancestors = 92069803)

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

You're probably right.

I was hoping there'd be a bit or something on the normal exit variant, but presuming we're generating callfinallys in IL order we'd see all the early leaves before the final one.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Looking at impImportLeave, the callfinally is placed via fgNewBBinRegion with the hint block set to either the step block, the block with the leave opcode, or null.

Because we're processing IL in lexical order, this ends up prepending callfinallies, so the first callfinally really does correspond to the fallthrough return path.

Oddly when I choose the last callfinally as the one to retarget, I get slightly smaller code overall. Still investigating why.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Looks like there are some examples, like System.Text.StringBuilder:AppendJoinCore(long,int,ref):ref:this (source), where using creates a try-finally with several exits, and clone placement using either first or last callfinally have odd layout issues. Will dig in some more.

// some of them may only be reachable via an exceptional path.
BasicBlock* firstCallFinallyBlock = nullptr;
BasicBlock* lastCallFinallyBlock = nullptr;
ehGetCallFinallyBlockRange(XTnum, &firstCallFinallyBlock, &lastCallFinallyBlock);

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 13, 2016

Member

ehGetCallFinallyBlockRange()

For all calls to ehGetCallFinallyBlockRange, you need to name the third argument endCallFinallyBlock, or anything that doesn't contain the name last. There is a longstanding convention (and confusion) with the EH table that "last" points to the last block of a range, and "end" points one past that (used for iteration). By naming this param "last", you are confusing the two.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

Will do, thanks.

BasicBlock* normalCallFinallyBlock = nullptr;
BasicBlock* normalCallFinallyReturn = nullptr;
BasicBlock* cloneInsertAfter = lastCallFinallyBlock;

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 13, 2016

Member

Seems like you'd want to initialize cloneInsertAfter to the last block of the try region, so HBtab->ebdTryLast. It will get overridden below in the FEATURE_EH_CALLFINALLY_THUNKS case. But really it needs to use fgNewBBinRegion.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 13, 2016

Member

As is, the insert-after point is the block after the last of the try region.


In reply to: 92071336 [](ancestors = 92071336)

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Dec 13, 2016

Author Member

My understanding was that when there were no call finally thunks, the callfinally and paired return would be within the try region, and (presuming we successfully identified the normal exit pair) would be right at the end of the region. So the placement hint puts the cloned finally just after the try, since that's the first place with an appropriate enclosing region.

Easy enough to change this hint to be to the try region end, if it seems clearer that way.

First block of the clone always uses fgNewBBinRegion with the enclosing try region, no matter what we use for a hint block. The main concern with the hint is to try and avoid jumping in and out of the cloned finally; if it's placed poorly nothing later seems to be able to fix it.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 13, 2016

Member

Yes, but ehGetCallFinallyBlockRange() sets its "end" / third argument to ebdTryLast->bbNext, not ebdTryLast. So I'm guessing that fgNewBBinRegion taking that as the hint might insert it after this, leaving at least one unwanted block the in middle.


// Modify the targeting callfinallies to branch to the cloned
// finally. Make a note if we see some calls that can't be
// retartged (since they want to return to other places).

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Dec 13, 2016

Member

retartged [](start = 11, length = 9)

typo: retartged

void fgRemoveEmptyFinally();

void fgCloneFinally();

This comment has been minimized.

Copy link
@briansull

briansull Jan 4, 2017

Contributor

Has the source code for the bodies of these two methods already been checked in?
If so maybe a reference to the PR# for them could be mentioned.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 4, 2017

Author Member

The code is in this PR. It may be github is hiding flowgraph.cpp changes from you (since they are large).

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 10, 2017

@briansull is this the kind of thing you had in mind? If so, I will push it up.

#if defined(DEBUG)
#if defined(FEATURE_CORECLR)
CONFIG_INTEGER(JitEnableFinallyCloning, W("JitEnableFinallyCloning"), 1)
#else
CONFIG_INTEGER(JitEnableFinallyCloning, W("JitEnableFinallyCloning"), 0)
#endif // defined(FEATURE_CORECLR)
#endif // DEBUG

// ------------

void Compiler::fgCloneFinally()
{ 
    JITDUMP("\n*************** In fgCloneFinally()\n");

#if FEATURE_CORECLR
    bool enableCloning = true;
#else
    // Finally cloning currently doesn't provide sufficient protection
    // for the cloned code in the presence of thread abort.
    bool enableCloning = false;
#endif // FEATURE_CORECLR

#if DEBUG
    // Allow override to enable/disable.
    enableCloning = (JitConfig.JitEnableFinallyCloning() == 1);
#endif // DEBUG

    if (!enableCloning)
    {
        JITDUMP("Finally cloning disabled.\n");
        return;
    }

//------------------------------------------------------------------------
// fgRemoveEmptyFinally: Remove try/finallys where the finally is empty
//

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 10, 2017

Member

It might be useful to highlight in the function header what the current limitations are, e.g. only single basic block, not doing 'fault' regions, must contain GT_RETFILT, etc.

// Does not yet handle thread abort. The open issues here are how
// to maintain the proper description of the cloned finally blocks
// as a handler (for thread abort purposes), how to prevent code
// motion in our out of these blocks, and how to report this cloned

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 10, 2017

Member

our [](start = 16, length = 3)

typo: our => or

@BruceForstall

This comment has been minimized.

Copy link
Member

BruceForstall commented Jan 10, 2017

LGTM. I'll admit I didn't look as carefully as the first time, but it looks very good.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 10, 2017

Updates to address Brian and Bruce's latest feedback.

@Brucefo @briansull PTAL...

@@ -22492,6 +22492,13 @@ void Compiler::fgLclFldAssign(unsigned lclNum)
// Converts callfinally to a jump to the finally continuation.
// Removes the finally, and reparents all blocks in the try to the
// enclosing try or method region.
//
// Currently limited to trivially empty finallys: those with one basic
// block containing only single RETFILT statment. It is possible but

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 10, 2017

Member

typo: statment => statement

// block containing only single RETFILT statment. It is possible but
// not likely that more complex-looking finallys will eventually become
// empty (from say subsequent optimization). An SPMI run with the
// just the "detection" part of this phase run after optimization

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 10, 2017

Member

grammar-o: "with the just the"

@BruceForstall

This comment has been minimized.

Copy link
Member

BruceForstall commented Jan 10, 2017

@AndyAyersMS LGTM, minor typo/grammar nits

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 10, 2017

These latest failures are unexpected, given that the only delta between this and the prior run were comment changes. Will see if I can repro locally.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 10, 2017

No local repro, so as much as I hate doing this, let's see if the lab can repro the failures.

@dotnet-bot retest Windows_NT x64 Release Priority 1 Build and Test

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 10, 2017

Actually I may not have run the full set of Pri1 tests locally, so retrying that locally too.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 11, 2017

No local repro and no lab repro. Am going to rebase/squash and put this through one more lab cycle.

JIT: Finally Optimizations
Adds two optimization for try-finallys: empty finally removal and
finally cloning.

Empty finally removal identifies trivially empty finally clauses and
removes the entire try-finally EH region. COde in the try is "promoted"
to be in the parent EH region (or method region). Empty finallys often
appear after inlining empty Dispose methods. Removing a try-finally with
an empty finally both reduces code size and improves code speed.

Finally cloning duplicates the code for the finally and 'inlines' it
along one of the normal exit paths from the try. This improves code
speed in the typical case where there is no exception raised while
the try is active. It generally increases code size slightly. However,
finallys are rare enough that the overall code size increase across
all methods is quite small. The jit will clone most finallys, provided
they are not too large, and are not contained in or contain other EH
constructs. If a try contains multiple exit paths only the final "fall
through" path will be optimized.

These optimizations are enabled for all target architectures. Finally
cloning is currently disabled for desktop CLR because more work is needed
to support thread abort.

More details on both optimizations can be found in the design document
added as part of this commit.

In debug builds, finally cloning can be selectively disabled or enabled
by setting COMPlus_JitEnableFinallyCloning to 0 or 1 respectively. This
config setting can thus be used override the default behavior (cloning
enabled for CoreCLR, disabled otherwise) for diagnostic or testing purposes.

Closes #1505. Closes #8065.

@AndyAyersMS AndyAyersMS force-pushed the AndyAyersMS:FinallyCloning branch from f133df8 to f2a2d9e Jan 11, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 11, 2017

@dotnet-bot test Windows_NT corefx_baseline

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 11, 2017

Hmm, another unexpected failure. Let's see if it repros....

@dotnet-bot retest Linux ARM Emulator Cross Debug Build

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 11, 2017

From what I can tell, the various CoreFX failures above are unrelated to this change.

When running CoreFX tests vs a checked CoreCLR, the BCL asserts in the core library are enabled, and there are 3 CoreFX test suites that hit BCL asserts: System.Linq.Expression, System.Runtime, and System.Reflection.Emit.

For instance in the System.Expression.Linq tests, one test asks for a non-default stack size and this triggers an assert:

        // CoreFX/src/System.Linq.Expressions/tests/CompilerTests.cs
        [Theory, ClassData(typeof(CompilationTypes))]
        public static void CompileDeepTree_NoStackOverflowFast(bool useInterpreter)
        {
            ...
            // Request a stack size of 1 to get the minimum size allowed.
            // This reduces the size of tree needed to risk a stack overflow.
            // This though will only risk overflow once, so the outerloop test
            // above is still needed.
            Thread t = new Thread(() => f = Expression.Lambda<Func<int>>(e).Compile(useInterpreter), 1);
            ...
         }

        // CoreCLR/src/mscorlib/src/System/Thread/Thread.cs
        private void SetStartHelper(Delegate start, int maxStackSize)
        {
            // We only support default stacks in CoreCLR
            Debug.Assert(maxStackSize == 0);

The other suite failures seem to be clustered around enums.

@RussKeldorph @stephentoub I assume we'd like to be able to cleanly run CoreFX tests vs debug/checked corelib? If so there is evidently some work to do.

If our main interest is really in testing the jit we should try dropping a checked jit dll into the release CoreCLR and then using that to test CoreFX. For instance here I was hoping I could get an additional check that finally cloning wouldn't break CoreFX.

@RussKeldorph

This comment has been minimized.

Copy link
Member

RussKeldorph commented Jan 11, 2017

@AndyAyersMS I'm actively working to get these (and all jobs) green. They have improved dramatically since last week. Would prefer avoiding the additional complexity of mixing checked/release, but that's certainly an option if deemed necessary.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 11, 2017

Is there a tracking issue for this anywhere? It would be helpful to know what failures are "known" failures.

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 11, 2017

LGTM

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Jan 11, 2017

Ok, thanks. Merging...

@AndyAyersMS AndyAyersMS merged commit c3673af into dotnet:master Jan 11, 2017

13 of 15 checks passed

Windows_NT x64 Checked Build and Test (Jit - CoreFx ) Build finished.
Details
Windows_NT x86 Checked Build and Test (Jit - CoreFx ) Build finished.
Details
CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details

@AndyAyersMS AndyAyersMS deleted the AndyAyersMS:FinallyCloning branch Jan 11, 2017

@AndyAyersMS AndyAyersMS referenced this pull request Aug 21, 2017

Merged

Remove empty try's #13493

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.