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 issue #23943 #24011

Merged
merged 2 commits into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@echesakovMSFT
Copy link
Member

echesakovMSFT commented Apr 15, 2019

The issue #23943 is due to literal 0xFFFFFFFFFFFFF000LL being long long unsigned int and using as if it were long long int.

coreclr/src/jit/emitarm64.cpp

Lines 8425 to 8427 in 77a5f9c

ssize_t relPageAddr =
(((ssize_t)dstAddr & 0xFFFFFFFFFFFFF000LL) - ((ssize_t)srcAddr & 0xFFFFFFFFFFFFF000LL)) >> 12;
dst = emitOutputShortAddress(dst, INS_adrp, IF_DI_1E, relPageAddr, dstReg);

All the following expressions

  • 0xFFFFFFFFFFFFF000LL
  • ((ssize_t)dstAddr & 0xFFFFFFFFFFFFF000LL)
  • ((ssize_t)srcAddr & 0xFFFFFFFFFFFFF000LL)
  • (((ssize_t)dstAddr & 0xFFFFFFFFFFFFF000LL) - ((ssize_t)srcAddr & 0xFFFFFFFFFFFFF000LL))

have long long unsigned int type (see https://godbolt.org/z/Z443bf).

Consequently, C++ compiler uses lsr instruction instead of asr instruction for shifting 12 bits to the right.

This can be seen from the generated assembly for the expression computing relPageAddr

  244978:       9274ce88        and     x8, x20, #0xfffffffffffff000
  24497c:       9274cec9        and     x9, x22, #0xfffffffffffff000
  244980:       cb090108        sub     x8, x8, x9
  244984:       d34cfd04        lsr     x4, x8, #12

With this changes (introducing and using computeRelPageAddr function) the corresponding expression for relPageAddr will be

  244978:       d34cfe88        lsr     x8, x20, #12
  24497c:       cb563104        sub     x4, x8, x22, lsr #12

Fixes issue #23943

Fix an issue with literal 0xFFFFFFFFFFFFF000LL being treated as unsig…
…ned long long int in src/jit/emitarm64.h src/jit/emitarm64.cpp
@echesakovMSFT

This comment has been minimized.

Copy link
Member Author

echesakovMSFT commented Apr 15, 2019

@echesakovMSFT echesakovMSFT added this to the 3.0 milestone Apr 15, 2019

@echesakovMSFT echesakovMSFT self-assigned this Apr 15, 2019

@sandreenko
Copy link
Member

sandreenko left a comment

LGTM, thanks for the improved readability.

I do not like any magic constants even if I know what they mean, but I am OK with leaving it as is because the function is small and has a comment.

@CarolEidt
Copy link
Member

CarolEidt left a comment

LGTM

@echesakovMSFT echesakovMSFT merged commit f6938e2 into dotnet:master Apr 16, 2019

12 of 14 checks passed

Ubuntu arm Cross Checked crossgen_comparison Build and Test Started.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Started.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Checked Innerloop Build and Test Build finished.
Details
Windows_NT arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details

@echesakovMSFT echesakovMSFT deleted the echesakovMSFT:GitHub23943 branch Apr 16, 2019

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.