Skip to content

Fix SuperPMI assertion call in MethodContext::recGetHelperFtn() - #90778

Merged
BruceForstall merged 2 commits into
dotnet:mainfrom
BruceForstall:FixSpmiAssert
Aug 18, 2023
Merged

Fix SuperPMI assertion call in MethodContext::recGetHelperFtn()#90778
BruceForstall merged 2 commits into
dotnet:mainfrom
BruceForstall:FixSpmiAssert

Conversation

@BruceForstall

Copy link
Copy Markdown
Contributor

We can't use string concatenation in an argument to the AssertCodeMsg macro, so construct the string we want to print first.

We can't use string concatenation in an argument to the `AssertCodeMsg`
macro, so construct the string we want to print first.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 18, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

We can't use string concatenation in an argument to the AssertCodeMsg macro, so construct the string we want to print first.

Author:BruceForstall
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@BruceForstall

Copy link
Copy Markdown
ContributorAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@BruceForstall

Copy link
Copy Markdown
ContributorAuthor

Related: #90711

Comment on lines +2367 to +2371
// We can't use string concatenation in an argument to the `AssertCodeMsg` macro, so construct the string here.
char assertMsgBuff[200];
sprintf_s(assertMsgBuff, sizeof(assertMsgBuff), "old: %016" PRIX64 " %016" PRIX64 ", new: %016" PRIX64 " %016" PRIX64,
oldValue.A, oldValue.B, value.A, value.B);
AssertCodeMsg(oldValue.A == value.A && oldValue.B == oldValue.B, EXCEPTIONCODE_MC, "collision! %s", assertMsgBuff);

@jakobbotschjakobbotschAug 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should just fix AssertCodeMsg like this:

- LogException(exCode, "SuperPMI assertion '%s' failed (" #msg ")", #expr, ##__VA_ARGS__);+ LogException(exCode, "SuperPMI assertion '%s' failed (" msg ")", #expr, ##__VA_ARGS__);

Currently it is stringizing the format string, which is almost certainly not correct.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense. Updated.

@EgorBo

EgorBo commented Aug 18, 2023

Copy link
Copy Markdown
Member

Maybe it should be just removed as in #90711 (comment) ? Presumably the same assert in many other JIT-EE functions will fail too (but they don't have it)

@BruceForstall

Copy link
Copy Markdown
ContributorAuthor

Maybe it should be just removed...

Removing the assert in recGetHelperFtn is a separate question. This PR just fixes how the asserts are implemented.


AssertCodeMsg(oldValue.A == value.A && oldValue.B == oldValue.B, EXCEPTIONCODE_MC,
"collision! old: %016" PRIX64 " %016" PRIX64 ", new: %016" PRIX64 " %016" PRIX64 " \n", oldValue.A, oldValue.B, value.A,
"collision! old: %016" PRIX64 " %016" PRIX64 ", new: %016" PRIX64 " %016" PRIX64, oldValue.A, oldValue.B, value.A,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Asserts shouldn't have newlines in them

@BruceForstall
BruceForstall merged commit 028ad32 into dotnet:mainAug 18, 2023
@BruceForstall
BruceForstall deleted the FixSpmiAssert branch August 18, 2023 18:36
@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@BruceForstall@EgorBo@jakobbotsch