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

Fix unreached during dump on amd64 Unix. #54861

Merged
merged 1 commit into from
Jun 28, 2021
Merged

Conversation

sandreenko
Copy link
Contributor

To repro the issue run the following with JitDump enabled:

        struct SMultiRegPromotedMatching
        {
            long l1;
            long l2;
        }


        [MethodImpl(MethodImplOptions.NoInlining)]
        static SMultiRegPromotedMatching Return1(SMultiRegPromotedMatching s)
        {
            return s;
        }

s is passed using 2 regs, so its type is VLT_REG_REG.

@sandreenko sandreenko added good first issue Issue should be easy to implement, good for first-time contributors arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jun 28, 2021
@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib, it is a one line change to fix JitDump issue.

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

@BruceForstall
Copy link
Member

The declaration of VLT_REG_REG says:

    // VLT_REG_REG -- TYP_LONG with both uint32_ts enregistred
    // eg. RBM_EAXEDX

which doesn't correspond to your case. In fact, it only seems to be set under !TARGET_64BIT... unless I'm behind?

@sandreenko
Copy link
Contributor Author

The declaration of VLT_REG_REG says: TYP_LONG with both uint32_ts enregistred

Looks outdated, that is how we get it on x64 Unix:

void CodeGen::genFnProlog()->
void CodeGen::psiBegProlog()->
void CodeGen::psiBegProlog()->
void CodeGenInterface::siVarLoc::storeVariableInRegisters(regNumber reg, regNumber otherReg)

varLocation.storeVariableInRegisters(regNum, otherRegNum);

// Two register are used
vlType = VLT_REG_REG;
vlRegReg.vlrrReg1 = reg;
vlRegReg.vlrrReg2 = otherReg;
}
}

which doesn't correspond to your case. In fact, it only seems to be set under !TARGET_64BIT... unless I'm behind?

where do you see this?

Let me know if you want the comment to be updated.

@BruceForstall
Copy link
Member

I was looking at CodeGenInterface::siVarLoc::siFillRegisterVarLoc; I missed the one you point out.

Don't worry about updating the comment now.

However, I'm curious if the debugger actually works with this...

@sandreenko sandreenko merged commit 851eafb into dotnet:main Jun 28, 2021
@sandreenko sandreenko deleted the fixDump branch June 28, 2021 23:37
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 29, 2021
…bugger2

* origin/main: (78 commits)
  Fix unreached during dump. (dotnet#54861)
  Fix lowering usage of an unset LSRA field. (dotnet#54731)
  Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (dotnet#54786)
  Add perf_slow yaml (dotnet#54853)
  Faster type load for scenarios made more common by generic math (dotnet#54588)
  Make sure we consider buffer length when marshalling back Unicode ByValTStr fields (dotnet#54695)
  Add YieldProcessor implementation for arm (dotnet#54829)
  Remove ActiveIssue for dotnet#50968 (dotnet#54831)
  Enable System.Text.Json tests for Wasm AOT (dotnet#54833)
  Remove ActiveIssue for dotnet#51723 (dotnet#54830)
  Fix load exception on generic covariant return type (dotnet#54790)
  Obsolete X509Certificate2.PrivateKey and PublicKey.Key. (dotnet#54562)
  First round of converting System.Drawing.Common to COMWrappers (dotnet#54636)
  Fix alloc-dealloc mismatches (dotnet#54701)
  Add one-shot ECB methods
  [Mono] MSBuild Task housekeeping (dotnet#54485)
  Move iOS/tvOS simulator AOT imports in the Mono workload (dotnet#54821)
  Remove unnecessary char[] allocation from Uri.GetRelativeSerializationString (dotnet#54799)
  Reduce overhead of Enumerable.Chunk (dotnet#54782)
  Fix EnumMemberRefs always returning NULL (dotnet#54805)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI good first issue Issue should be easy to implement, good for first-time contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants