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

Add safety slot before variable for probe array #893

Merged
merged 10 commits into from May 15, 2023
Merged

Add safety slot before variable for probe array #893

merged 10 commits into from May 15, 2023

Conversation

Godin
Copy link
Member

@Godin Godin commented Jun 7, 2019

Fixes #767
Fixes #1412
Fixes #1429

@Godin Godin added type: bug 🐛 Something isn't working component: core labels Jun 7, 2019
@Godin Godin added this to the 0.8.5 milestone Jun 7, 2019
@Godin Godin self-assigned this Jun 7, 2019
@Godin Godin added this to Implementation in Current work items via automation Jun 7, 2019
@Godin Godin marked this pull request as ready for review June 7, 2019 01:44
@Godin Godin requested a review from marchof June 7, 2019 01:45
@Godin Godin moved this from Implementation to Review in Current work items Jun 7, 2019
@Godin Godin moved this from Review to Implementation in Current work items Aug 7, 2019
@Godin Godin removed this from the 0.8.5 milestone Oct 11, 2019
@Jacks0N23
Copy link

Jacks0N23 commented Apr 12, 2023

I test it locally and it fix my problem from this issue
Please, can you merge it?

@AlexeyKorshun
Copy link

can you merge this fix?

@Godin
Copy link
Member Author

Godin commented Apr 27, 2023

@Jacks0N23 @AlexeyKorshun

can you merge it?

unfortunately not yet - see #893 (comment)

@Godin Godin marked this pull request as draft April 27, 2023 10:17
@marchof
Copy link
Member

marchof commented May 3, 2023

@Godin Given the complexity of inserting the probe array somewhere "in the middle" we might consider simply adding it to the end. This will increase the byte code size because it will be less likely that ALOAD_0 ... ALOAD_3 can be used but has probably no performance impact. WDYT?

@Godin
Copy link
Member Author

Godin commented May 3, 2023

@marchof regarding #893 (comment)

has probably no performance impact

I'm not so sure, are you? 😉

This will increase the byte code size

And method size is limited 😉

simply adding it to the end

I wouldn't call it "simply" - ProbeInserter#visitMaxs can not be used to determine position, because it is invoked after other ProbeInserter visit methods (such as ProbeInserter#visitVarInsn) that already should know position, so we will need to find another place/way to compute position. Which to me sounds more complex than a modification of this PR that will fix #893 (comment), and especially since I already have this modification and will publish it shortly 😉

@Godin Godin force-pushed the issue-767 branch 2 times, most recently from ae8f2f2 to a74404c Compare May 3, 2023 21:46
@Godin Godin added this to the 0.8.11 milestone May 3, 2023
@Godin Godin marked this pull request as ready for review May 3, 2023 23:05
@Godin Godin moved this from Implementation to Review in Current work items May 3, 2023
@Godin
Copy link
Member Author

Godin commented May 3, 2023

@marchof I rebased this PR and fixed the problem described in #893 (comment), it still lacks in-code comments requested in #893 (comment) and the changelog entry, but maybe you already can review what else needs to be tweaked in the implementation?


Fun fact: the issue can be reproduced by using Kotlin compiler versions starting from released 7 years ago 1.0 🙈 😆 up to the latest as of today released 1.8.21 - enough to declare an inline function with a default value for a parameter and body that immediately declares a variable of Long or Double type

inline fun example(x: Int = 0): Double {
    val y: Double = 0.0
    return x + y
}

fun main(args: Array<String>) {
}

@Godin
Copy link
Member Author

Godin commented May 11, 2023

@marchof I added comments and updated changelog, so IMO this can be merged 🎉

@marchof marchof merged commit 1a1db3b into master May 15, 2023
23 checks passed
Current work items automation moved this from Review to Done May 15, 2023
@marchof marchof deleted the issue-767 branch May 15, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core type: bug 🐛 Something isn't working
Projects
4 participants