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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unsafe.Tag based type resolution for CFuncPtr.{fromScalaFunction,apply} #3270

Merged
merged 8 commits into from May 8, 2023

Conversation

tanishiking
Copy link
Member

@tanishiking tanishiking commented May 5, 2023

Fix #3220

This pull request refacts the handling of CFuncPtr.{fromscalaFunction,apply} by preserving the exact type information during the type erasure phase and referencing it in the NIR generation phase. This is the same approach as sizeOf[T] and alignmentOf[T] #3198.

Previously, we restored the type information based on the `unsafe.Tag' proof types, which is a bit buggy.

This PR includes the following changes

  • Addition of special attachments to the AST for CFuncPtr#apply and CFuncPtr.fromScalaFunction whose values are their type information (parameter types and return type).
  • At the NIR generation phase (NIRGenExpr.scala), get the attached exact type information and use it to generate external forwarder methods (previously we used the type information restored based on unsafe.Tag).
  • Remove the dead code for tag parsing in NirGenUtil.unwrap{ClassTag,Tag} and Tags related code in NirDefinitions.
  • Update nativelib/src/main/scala/scalanative/unsafe.CFuncPtr.scala.gyb to remove unsed evidences types.
finished todo

TODOs

  • Remove unsafe.Tag from CFuncPtrX and remove unwrapTag from NirGenUtil`
  • Migrate scalalib-patch-tool to scala-cli
    • It required to re-generate patch files to publish locally during development, but it turned out we don't need re-creation now 馃 Maybe we can separate f86daba to another PR.
  • Testing
    • tests2_13 / Test
    • test3 / Test
    • test2_12 / Test
      • auxlib failed to build with this patch nevermind, it seems like it my local environment probem.
  • manual testing (testing on https://github.com/tanishiking/scala-native-sbt)
    • ++3.2.2 publishLocal ok
    • ++2.12.17 publishLocal
    • ++2.13.10 publishLocal~ it seems like nativeLink always fail with the following error while tests pass.~ it's also my local environment problem. nevermind
[info] ZipFile invalid LOC header (bad signature)
[error] Found 2 missing definitions while linking
[error] Not found Top(Main)
[error] Not found Member(Top(Main),D4mainLAL16java.lang.String_uEo)
[error] Undefined definitions found in reachability phase
[error] (Compile / nativeLink) Undefined definitions found in reachability phase
[error] Total time: 1 s, completed May 5, 2023, 10:11:12 PM

Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

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

Looks good so far, I've left some comments how we can simplify it a bit.

Regarding to problems with local publishing, it is probably due to usage of nativeLink task in root project. I'm not sure where does it comes from, probably we could inspect the build and override it. Due to complexity of the build (we don't have typical crossVersion but use project duplicates for each Scala binary version), and the fact that there are cases where we need to either always depend on Scala 2.12 (sbtPlugin) or Scala 2.13 (scalalib for Scala 3) running tasks in root project is not recommended. To target a single project you can use one of dedicated subprojects, eg. tests2_13, tests3, etc.
If you want to publish all projects locally you can use a custom command publish-local-dev <scala-version>

scripts/scalalib-patch-tool.sc Outdated Show resolved Hide resolved
Comment on lines 215 to 224
case Apply(TypeApply(fun, tArgs), args) if CFuncPtrFromFunctionMethods.contains(fun.symbol) =>
val idx = CFuncPtrFromFunctionMethods.indexOf(fun.symbol)
val transformed = _CFuncPtrFromFunctionMethods(idx)
val tys = tArgs.map(t => widenDealiasType(t.tpe))
typer
.typed {
Apply(transformed, args: _*)
}
.updateAttachment(NonErasedTypes(tys))
.setPos(tree.pos)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this attachment is not used anywhere (at least in Scala2), we use only attachment for CFuncPtrApply. It means to can safely remove handling fromScalaFunction and it's erased variant _fromScalaFunction.

It made me think about how we can achiv the same in Scala 3. I'll describe below how to get rid of this attachment for Scala3 version of this method

Comment on lines 57 to 67
case Apply(TypeApply(fun, tArgs), args) if defnNir.CFuncPtr_fromScalaFunction.contains(fun.symbol) =>
val idx = defnNir.CFuncPtr_fromScalaFunction.indexOf(fun.symbol)
val transformed = defnNir._CFuncPtr_fromScalaFunction(idx)
val cls = defnNir.CFuncPtrNClass(idx)
val tys = tArgs.map(t => dealiasTypeMapper(t.typeOpt))
cpy
.Apply(tree)(
ref(transformed),
args
)
.withAttachment(NirDefinitions.NonErasedTypes, tys)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add attachment to outer Apply without transformation of inner TypeApply. This combined with the fact that on Scala 2 it's not needed as well, would allow to eliminate additional _fromScalaFunction method.
In Scala2 this transformation was unused, but in Scala3 we need to get original type and save it in attachment, because the target of a lambda might get replaced with adapted (boxed and erased) variant of a function. https://github.com/lampepfl/dotty/blob/abf9a25d5f27a41c11c11c11cf671ed3cb33d10d/compiler/src/dotty/tools/dotc/transform/Erasure.scala#L409-L439

Suggested change
case Apply(TypeApply(fun, tArgs), args) if defnNir.CFuncPtr_fromScalaFunction.contains(fun.symbol) =>
val idx = defnNir.CFuncPtr_fromScalaFunction.indexOf(fun.symbol)
val transformed = defnNir._CFuncPtr_fromScalaFunction(idx)
val cls = defnNir.CFuncPtrNClass(idx)
val tys = tArgs.map(t => dealiasTypeMapper(t.typeOpt))
cpy
.Apply(tree)(
ref(transformed),
args
)
.withAttachment(NirDefinitions.NonErasedTypes, tys)
case appy @ Apply(TypeApply(fun, tArgs), _) if defnNir.CFuncPtr_fromScalaFunction.contains(fun.symbol) =>
val tys = tArgs.map(t => dealiasTypeMapper(t.typeOpt))
app
.withAttachment(NirDefinitions.NonErasedTypes, tys)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! _fromScalaFunction was a thing I don't like in this change and I'm glad we can get rid of that!
done cc62ca2

but in Scala3 we need to get original type and save it in attachment, because the target of a lambda might get replaced with adapted (boxed and erased) variant of a function.

Oh, that's why we didn't need this in Scala2, while Scala3 does. That makes sense, thank you for the explanation!

scripts/scalalib-patch-tool.sc Outdated Show resolved Hide resolved
implicit def fromScalaFunction[${allTps}](fn: ${FunctionN})(implicit ${evidences}): ${CFuncPtrN} = intrinsic
implicit def fromScalaFunction[${allTps}](fn: ${FunctionN}): ${CFuncPtrN} = intrinsic

def _fromScalaFunction(fn: ${FunctionNWildcard}): ${CFuncPtrNWildcard} = intrinsic
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get rid of these, see comments below

This reverts commit f86daba.

Reverting this change for now because it's not related to the
pull request that refactor scala-native not to use
unsafe.Tag based type preservation.
@tanishiking
Copy link
Member Author

tanishiking commented May 8, 2023

Regarding to problems with local publishing, it is probably due to usage of nativeLink task in root project. I'm not sure where does it comes from, probably we could inspect the build and override it

If you want to publish all projects locally you can use a custom command publish-local-dev <scala-version>

Regarding the auxlib, it seems like the same problem as #3265 and I do workaround by publishing locally for another version of scala 2.12

It seems like the linking problems are caused by my contaminated local environment (by ++x.x.x publishLocal 馃槄), removing the local cache, and republish by publish-local-dev x.x.x fixed the problem. Thanks!

@tanishiking tanishiking changed the title [WIP] Remove unsafe.Tag based type resolution for CFuncPtr.{fromScalaFunction,apply} Remove unsafe.Tag based type resolution for CFuncPtr.{fromScalaFunction,apply} May 8, 2023
@tanishiking tanishiking marked this pull request as ready for review May 8, 2023 06:34
tanishiking added a commit to tanishiking/scala-native that referenced this pull request May 8, 2023
There's no longer a special scripts like `scripts/publish-local`.
ScalaNative is required to depend on multiple Scala versions
e.g. Scala 2.12 (sbtPlugin) or Scala 2.13 (scalalib for Scala 3)
and just running `++x.y.z publishLocal` on root project is not
recommended.
Instead, we can use `publish-local-dev` custom task in sbt.

see: scala-native#3270 (review)
tanishiking added a commit to tanishiking/scala-native that referenced this pull request May 8, 2023
There's no longer a special scripts like `scripts/publish-local`.
ScalaNative is required to depend on multiple Scala versions
e.g. Scala 2.12 (sbtPlugin) or Scala 2.13 (scalalib for Scala 3)
and just running `++x.y.z publishLocal` on root project is not
recommended.
Instead, we can use `publish-local-dev` custom task in sbt.

see: scala-native#3270 (review)
WojciechMazur pushed a commit that referenced this pull request May 8, 2023
There's no longer a special scripts like `scripts/publish-local`.
ScalaNative is required to depend on multiple Scala versions
e.g. Scala 2.12 (sbtPlugin) or Scala 2.13 (scalalib for Scala 3)
and just running `++x.y.z publishLocal` on root project is not
recommended.
Instead, we can use `publish-local-dev` custom task in sbt.

see: #3270 (review)
@WojciechMazur WojciechMazur merged commit a349273 into scala-native:main May 8, 2023
68 checks passed
@tanishiking tanishiking deleted the remove-tags-cfuncptr branch May 8, 2023 13:25
WojciechMazur pushed a commit that referenced this pull request May 23, 2023
There's no longer a special scripts like `scripts/publish-local`.
ScalaNative is required to depend on multiple Scala versions
e.g. Scala 2.12 (sbtPlugin) or Scala 2.13 (scalalib for Scala 3)
and just running `++x.y.z publishLocal` on root project is not
recommended.
Instead, we can use `publish-local-dev` custom task in sbt.

see: #3270 (review)
(cherry picked from commit 713593a)
WojciechMazur pushed a commit that referenced this pull request Jun 5, 2023
There's no longer a special scripts like `scripts/publish-local`.
ScalaNative is required to depend on multiple Scala versions
e.g. Scala 2.12 (sbtPlugin) or Scala 2.13 (scalalib for Scala 3)
and just running `++x.y.z publishLocal` on root project is not
recommended.
Instead, we can use `publish-local-dev` custom task in sbt.

see: #3270 (review)
(cherry picked from commit 713593a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFuncPtr.{fromScalaFunction,apply} should not require scalanative.unsafe.Tag
3 participants