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

cmd/compile: mark pointer types pointing to noalg types noalg too #47068

Closed
martisch opened this issue Jul 6, 2021 · 4 comments
Closed

cmd/compile: mark pointer types pointing to noalg types noalg too #47068

martisch opened this issue Jul 6, 2021 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@martisch
Copy link
Contributor

martisch commented Jul 6, 2021

Similar problem to #32595.

Found this bug while investigating avoiding to create algs for array types that are only used as backing array for slices.
It seems this is the reason our initial attempts at introducing that optimisation in https://go-review.googlesource.com/c/go/+/151497 failed with test errors.

Arrays marked noalg are created by the compiler to hold keys and values
to initialize map literals. The ssa backend creates a pointer type for the
array type when creating an OpAddr while processing the loop that
initializes the map from the arrays. The pointer type does not inherit
the noalg property but points to the noalg array type.

This leads to strange problems were the same values of a type dont compare equal anymore if created through reflect:

package a

func A() {
   var m map[int]int = map[int]int{
      0: 0, 1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0, 7: 0, 8: 0, 9: 0,
      10: 0, 11: 0, 12: 0, 13: 0, 14: 0, 15: 0, 16: 0, 17: 0, 18: 0, 19: 0,
      20: 0, 21: 0, 22: 0, 23: 0, 24: 0, 25: 0, 26: 0, 27: 0, 28: 0, 29: 0}
   if len(m) != 30 {
      panic("unepexted map length")
   }
}
package b

import "reflect"

func B() {
   t1 := reflect.TypeOf([30]int{})
   t2 := reflect.TypeOf(new([30]int)).Elem()
   if t1 != t2 {
      panic("[30]int types do not match")
   }
}
package main

import (
   "a"
   "b"
)

func main() {
   a.A()
   b.B()
}
@martisch martisch self-assigned this Jul 6, 2021
@martisch martisch added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 6, 2021
@gopherbot
Copy link

Change https://golang.org/cl/333030 mentions this issue: cmd/compile: mark type of pointer to noalg type noalg too

@mdempsky
Copy link
Member

mdempsky commented Jul 7, 2021

This appears to be a pre-existing issue since at least Go 1.16.5: https://play.golang.org/p/DFjkXhMtEzO

This is an unfortunate issue, and it should be fixed. But I think fixing it will be too invasive for Go 1.17.

As mentioned on CL 333030, I think the right fix here is to teach the linker to realize alg and noalg type descriptors can be deduplicated, but to prefer the alg variants when present. There's no need for alg and noalg types to have different descriptors; noalg types only exist as a space optimization to avoid creating the equality and hash functions for generated types that we know will never need those. And we can save even more space by getting rid of the redundant noalg variants when they're not needed.

(I'm sure there's some trickiness with dynamic linking here too. If we don't already, maybe we should just turn off noalg for dynamic linking. I imagine there are / will be similar issues there, and I think we have less control over symbol definition priority in the OS's dynamic linker.)

@gopherbot
Copy link

Change https://golang.org/cl/344349 mentions this issue: cmd/compile: mark type of pointer to noalg type noalg too

@gopherbot
Copy link

Change https://golang.org/cl/344349 mentions this issue: cmd/compile: do not mark arrays used for map initialization noalg

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants