8000 Fix #4098: Avoid `let`/`const` when building the ASTs for GCC. by sjrd · Pull Request #4110 · scala-js/scala-js · GitHub
[go: up one dir, main page]
Skip to content

Fix #4098: Avoid let/const when building the ASTs for GCC. #4110

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Fix #4098: Avoid let/const when building the ASTs for GCC. #4110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Fix #4098: Avoid let/const when building the ASTs for GCC. 
We now use `var`s instead, which do not expose the issue.

This is a workaround for an unknown root cause. We should properly
fix this in the future.
  • Loading branch information
@sjrd
sjrd committed Jul 1, 2020
commit 42dd238b510db0ae955a713911f7b2db50691021
12 changes: 11 additions & 1 deletion linker/jvm/src/main/scala/org/scalajs/linker/backend/closure/ClosureAstTransformer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,17 @@ private class ClosureAstTransformer(featureSet: FeatureSet,
case Let(ident, mutable, optRhs) =>
val node = transformName(ident)
optRhs.foreach(rhs => node.addChildToFront(transformExpr(rhs)))
new Node(if (mutable) Token.LET else Token.CONST, node)
/* #4098 The following line should be
* new Node(if (mutable) Token.LET else Token.CONST, node)
* however, due to an unknown bug in how we build GCC trees or inside
* GCC itself, using `let`s and `const`s can result in a crash deep
* inside GCC in some obscure cases.
* As a workaround, we always emit `var`s instead. This has no visible
* semantic change because the Emitter never relies on the specific
* semantics of `let`s and `const`s.
* TODO We should properly fix this at the root (filed as #4109).
*/
new Node(Token.VAR, node)
case Skip() =>
new Node(Token.EMPTY)
case Block(stats) =>
Expand Down
7 changes: 7 additions & 0 deletions test-suite/shared/src/test/scala/org/scalajs/testsuite/compiler/RegressionTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,13 @@ class RegressionTest {
assertEquals(6, `class`.foo(5))
}

@Test def gcc_crash_with_let_const_issue_4098(): Unit = {
val set = new java.util.HashSet[String]()
set.remove("")
set.remove("1") // only if remove is called twice
assertEquals(0, set.size())
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fail in the test suite without the fix? (i.e. does it fail not in isolation?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does. I had checked ;)

}

}

object RegressionTest {
Expand Down
0