Skip to content

Add jvm backend check for UTF-8 Constant length #19622

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ class Compiler {
new sjs.JUnitBootstrappers, // Generate JUnit-specific bootstrapper classes for Scala.js (not enabled by default)
new CollectEntryPoints, // Collect all entry points and save them in the context
new CollectSuperCalls, // Find classes that are called with super
new RepeatableAnnotations) :: // Aggregate repeatable annotations
new RepeatableAnnotations, // Aggregate repeatable annotations
new CheckJVMSpecification) :: // Checks related to the JVM Specification before code generation
Nil

/** Generate the output of the compilation */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package dotty.tools.dotc
package transform

import MegaPhase.MiniPhase
import GenericSignatures.*
import core.Contexts.*
import core.Decorators.*
import core.Constants.*
import core.Symbols.*
import util.SrcPos

final class CheckJVMSpecification extends MiniPhase:
Copy link
Member

Choose a reason for hiding this comment

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

Consider an end CheckJVMSpecification marker for this class. It spans more than one screen.

import ast.tpd.*

override def phaseName: String = CheckJVMSpecification.name
override def description: String = CheckJVMSpecification.description


override def transformDefDef(tree: DefDef)(using Context): Tree =
val sym = tree.symbol
val memberTpe = atPhase(ctx.base.erasurePhase) { sym.owner.denot.thisType.memberInfo(sym) }
for sig <- GenericSignatures.javaSig(sym, memberTpe) do
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we'll compute the Java generic signature twice for every DefDef? Once here and once in the actual back-end? That's not great; it's a somewhat expensive operation.

checkUTF8MaximumLength(sig, tree.srcPos, prefix = "Method signature")
tree

override def transformLiteral(tree: Literal)(using Context): Tree =
val Literal(cste) = tree
if cste.tpe == defn.StringType then
checkUTF8MaximumLength(cste.stringValue, tree.srcPos.focus, prefix = "String")
tree

/** See https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4.7 */
private def checkUTF8MaximumLength(str: String, pos: SrcPos, prefix: String)(using Context) =
val UTF8_MAX_LENGTH = 65535
if str.length > UTF8_MAX_LENGTH then
Comment on lines +34 to +35
Copy link
Member

Choose a reason for hiding this comment

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

Huh, so this is not precise enough. It will work for ASCII strings without null characters, but not for all strings. The referenced spec says that the u2 length field is the number of bytes of the encoded string. But null chars and non-ASCII chars take more than 1 byte (up to 6!).

We should simulate the encoding and count the number of bytes actually taken. As a fast-path, we should succeed early if str.length <= (UTF8_MAX_LENGTH / 6), since that is going the overwhelmingly common case.

report.error(em"""${prefix} length exceed the maximum length allowed by the JVM specification.
|Allowed: ${UTF8_MAX_LENGTH}
|Actual : ${str.length}""", pos)

object CheckJVMSpecification:
val name = "checkJVMspec"
val description = "check issues related to the jvm specification"

6 changes: 6 additions & 0 deletions tests/neg/i15850-a.check

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions tests/neg/i15850-a.scala

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions tests/neg/i15850-b.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- Error: tests/neg/i15850-b.scala:9:4 ---------------------------------------------------------------------------------
9 |def crash(x: Level7): Unit = () // error
|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|Method signature length exceed the maximum length allowed by the JVM specification.
|Allowed: 65535
|Actual : 496979
Copy link
Contributor

Choose a reason for hiding this comment

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

it's an awesome error.

9 changes: 9 additions & 0 deletions tests/neg/i15850-b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
type Level1 = Tuple4[Unit, Unit, Unit, Unit]
type Level2 = Tuple4[Level1, Level1, Level1, Level1]
type Level3 = Tuple4[Level2, Level2, Level2, Level2]
type Level4 = Tuple4[Level3, Level3, Level3, Level3]
type Level5 = Tuple4[Level4, Level4, Level4, Level4]
type Level6 = Tuple4[Level5, Level5, Level5, Level5]
type Level7 = Tuple4[Level6, Level6, Level6, Level6]

def crash(x: Level7): Unit = () // error
1 change: 1 addition & 0 deletions tests/pos/i15850.scala

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion tests/printing/transformed/lazy-vals-legacy.check
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
[[syntax trees at end of MegaPhase{dropOuterAccessors, checkNoSuperThis, flatten, transformWildcards, moveStatic, expandPrivate, restoreScopes, selectStatic, Collect entry points, collectSuperCalls, repeatableAnnotations}]] // tests/printing/transformed/lazy-vals-legacy.scala
[[syntax trees at end of MegaPhase{dropOuterAccessors, checkNoSuperThis, flatten, transformWildcards, moveStatic, expandPrivate, restoreScopes, selectStatic, Collect entry points, collectSuperCalls, repeatableAnnotations, checkJVMspec}]] // tests/printing/transformed/lazy-vals-legacy.scala
package <empty> {
@SourceFile("tests/printing/transformed/lazy-vals-legacy.scala") final module
class A extends Object {
Expand Down
2 changes: 1 addition & 1 deletion tests/printing/transformed/lazy-vals-new.check
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
[[syntax trees at end of MegaPhase{dropOuterAccessors, checkNoSuperThis, flatten, transformWildcards, moveStatic, expandPrivate, restoreScopes, selectStatic, Collect entry points, collectSuperCalls, repeatableAnnotations}]] // tests/printing/transformed/lazy-vals-new.scala
[[syntax trees at end of MegaPhase{dropOuterAccessors, checkNoSuperThis, flatten, transformWildcards, moveStatic, expandPrivate, restoreScopes, selectStatic, Collect entry points, collectSuperCalls, repeatableAnnotations, checkJVMspec}]] // tests/printing/transformed/lazy-vals-new.scala
package <empty> {
@SourceFile("tests/printing/transformed/lazy-vals-new.scala") final module
class A extends Object {
Expand Down