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

order of annotations in produced bytecode is not deterministic #14743

Closed
raboof opened this issue Mar 22, 2022 · 5 comments · Fixed by #15063
Closed

order of annotations in produced bytecode is not deterministic #14743

raboof opened this issue Mar 22, 2022 · 5 comments · Fixed by #15063

Comments

@raboof
Copy link
Contributor

raboof commented Mar 22, 2022

This issue is not complete yet, but I wanted to get it out there before I lose track myself :)

Testing with Scala 3.1.1, it seems the same source code may lead to different orderings of annotations and imports in the resulting bytecode. For example for the following sources:

package akka.actor.typed.internal.jfr

import jdk.jfr.Category
import jdk.jfr.Enabled
import jdk.jfr.Event
import jdk.jfr.Label
import jdk.jfr.StackTrace

import akka.annotation.InternalApi

(... snip ...)

/** INTERNAL API */
@InternalApi
@Enabled(true)
@StackTrace(false)
@Category(Array("Akka", "Delivery", "ConsumerController")) @Label("Delivery ConsumerController producer changed")
final class DeliveryConsumerChangedProducer(val producerId: String) extends Event

we saw the following (decompiled-again with procyon -ec) bytecode on one compilation:

package akka.actor.typed.internal.jfr

import akka.annotation.InternalApi
import jdk.jfr.Enabled
import jdk.jfr.Label
import jdk.jfr.StackTrace
import jdk.jfr.Category
import jdk.jfr.Event

@Category({ "Akka", "Delivery", "ConsumerController" })
@StackTrace(false)
@Label("Delivery ConsumerController producer changed")
@Enabled(true)
@InternalApi
public final class DeliveryConsumerChangedProducer extends Event

but on another machine:

package akka.actor.typed.internal.jfr

import akka.annotation.InternalApi
import jdk.jfr.StackTrace
import jdk.jfr.Enabled
import jdk.jfr.Category
import jdk.jfr.Label
import jdk.jfr.Event

@Label("Delivery ConsumerController producer changed")
@Category({ "Akka", "Delivery", "ConsumerController" })
@Enabled(true)
@StackTrace(false)
@InternalApi
public final class DeliveryConsumerChangedProducer extends Event

I seem to remember seeing a similar issue in Scala 2, but can't find a reference right now.

I'll try and dig up further details, and test with a more recent version of scala3, later.

@raboof raboof added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 22, 2022
@TheElectronWill
Copy link
Contributor

Imports don't exist at the bytecode level, so that seems to be on procyon.
It's different for annotations, though. I see that java reflection API preserves their order. But does the order really matters?

@Kordyjan Kordyjan added area:backend stat:needs info and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 22, 2022
@raboof
Copy link
Contributor Author

raboof commented Mar 22, 2022

Imports don't exist at the bytecode level, so that seems to be on procyon.

Gotcha, sorry about that

It's different for annotations, though. I see that java reflection API preserves their order. But does the order really matters?

Perhaps not semantically (though I guess I could come up with some contrived examples), but in general it's attractive when the same source always produces the same 'bit-by-bit' bytecode: such 'reproducible builds' are useful for build systems that rely heavily on caching compiler artifacts and unlock some useful possibilities for security checks - there's some discussion around it in the context of scala 2 at scala/scala-dev#405, for example.

@raboof raboof changed the title order of annotations and imports in produced bytecode is not deterministic order of annotations in produced bytecode is not deterministic Mar 22, 2022
@smarter
Copy link
Member

smarter commented Mar 22, 2022

https://github.com/lampepfl/dotty/blob/main/tests/idempotency/IdempotencyCheck.scala checks that for all files in https://github.com/lampepfl/dotty/tree/main/tests/pos compilation is reproducible (it also checks that the order in which we compile files in sub-directories doesn't matter). You can run it manually with scala3-compiler/testOnly *IdempotencyTests, it'd be nice if you could reproduce the issue that way.

@smarter
Copy link
Member

smarter commented Mar 22, 2022

See also #7661

@raboof
Copy link
Contributor Author

raboof commented Mar 22, 2022

it'd be nice if you could reproduce the issue that way

A naive testcase:

import jdk.jfr.Enabled

@Deprecated
@Enabled
object Annotations {
}

doesn't reproduce the problem yet, so that needs some more digging.

raboof added a commit to raboof/dotty that referenced this issue Apr 28, 2022
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.

The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in scala#7661 and the corresponding scala/scala-dev#405

I tried adding an 'integration test' in `tests/pos` to be picked up by
`IdempotencyCheck.scala`, but unfortunately couldn't reproduce the
nondeterminism that way, so didn't end up including it in this commit.

I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.

This is basically the dotty equivalent of
scala/scala@954c5d3

Fixes scala#14743
raboof added a commit to raboof/dotty that referenced this issue Apr 28, 2022
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.

The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in scala#7661 and the corresponding scala/scala-dev#405

I tried adding an 'integration test' in `tests/pos` to be picked up by
`IdempotencyCheck.scala`, but unfortunately couldn't reproduce the
nondeterminism that way, so didn't end up including it in this commit.

I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.

This is basically the dotty equivalent of
scala/scala@954c5d3

Fixes scala#14743
raboof added a commit to raboof/dotty that referenced this issue Apr 28, 2022
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.

The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in scala#7661 and the corresponding scala/scala-dev#405

An 'integration test' `Annotations.scala` was added in `tests/pos`
to be picked up by `IdempotencyCheck.scala`, but unfortunately I
haven't successfully reproduced the nondeterminism that way.

I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.

This is basically the dotty equivalent of
scala/scala@954c5d3

Fixes scala#14743
raboof added a commit to raboof/dotty that referenced this issue Apr 28, 2022
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.

The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in scala#7661 and the corresponding scala/scala-dev#405

Some integration tests were added in `tests/pos` to be picked up by
`IdempotencyCheck.scala`, but unfortunately I haven't successfully
reproduced the nondeterminism that way.

I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.

This is basically the dotty equivalent of
scala/scala@954c5d3

Fixes scala#14743
raboof added a commit to raboof/dotty that referenced this issue Apr 28, 2022
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.

The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in scala#7661 and the corresponding scala/scala-dev#405

Some integration tests were added in `tests/pos` to be picked up by
`IdempotencyCheck.scala`, but unfortunately I haven't successfully
reproduced the nondeterminism that way.

I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.

This is basically the dotty equivalent of
scala/scala@954c5d3

Fixes scala#14743
bishabosha pushed a commit to dotty-staging/dotty that referenced this issue Oct 18, 2022
This change makes sure non-repeated annotations are kept in the order
they were found in the source code.

The motivation is not necessarily to have them in the original order,
but to have them in an order that is deterministic across rebuilds
(potentially even across different machines), for reasons discussed
further in scala#7661 and the corresponding scala/scala-dev#405

Some integration tests were added in `tests/pos` to be picked up by
`IdempotencyCheck.scala`, but unfortunately I haven't successfully
reproduced the nondeterminism that way.

I didn't see an obvious place for a 'unit test' of this code, I'd be
happy to add one when someone can recommend a good place to put it.

This is basically the dotty equivalent of
scala/scala@954c5d3

Fixes scala#14743
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants