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

Package's ".x" is not reproducible between dependency upgrade #4111

Open
sluongng opened this issue Sep 20, 2024 · 11 comments
Open

Package's ".x" is not reproducible between dependency upgrade #4111

sluongng opened this issue Sep 20, 2024 · 11 comments

Comments

@sluongng
Copy link
Contributor

This is a tracking issue for golang/go#69547

I was upgrading our external dependencies recently and these 2 GoCompilePkg actions ran on the same package between 2 consecutive commits.

Specifically, the package is server/util/db with a single source file db.go. The upgraded dependency was gorm.io/driver/{mysql,clickhouse,sqlite}. There was no code change in db.go.

The expectation is for db.x output to stay the same between the 2 compilations as no exported data was change. However, we observed that there were some bytes differences between the 2 .x files, which caused downstream compile actions to all rebuild.

~> diffoscope db{1,2}.x
--- db1.x
+++ db2.x
│┄ Command `'nm -s {}'` failed with exit code 1. Standard output:
│┄     /Library/Developer/CommandLineTools/usr/bin/nm: error: for the -s option: bad number of arguments (must be two arguments)
│┄     /Library/Developer/CommandLineTools/usr/bin/nm: error: a.out: No such file or directory
@@ -34357,15 +34357,15 @@
 00086340: 0104 0203 0405 1000 8108 0082 0803 7b03  ..............{.
 00086350: 4303 0103 0203 4103 8001 0324 034a 031b  C.....A....$.J..
 00086360: 03d4 0303 d503 03dc 0303 d803 0081 080f  ................
 00086370: 010d 0203 0405 0607 0809 0a0b 0c0d 0e0b  ................
 00086380: 0083 0800 8408 0343 034a 03d4 0303 d503  .......C.J......
 00086390: 03d8 0303 da03 0341 0365 0083 080a 0108  .......A.e......
 000863a0: 0203 0405 0607 0809 1200 8508 0086 0803  ................
-000863b0: bd01 03d4 0303 d804 037b 0341 0380 0103  .........{.A....
+000863b0: bd01 03d8 0403 d403 037b 0341 0380 0103  .........{.A....
 000863c0: 4a03 ea04 03d5 0303 d504 03d8 0303 2403  J.............$.
 000863d0: ef04 03d6 0403 dc03 0085 0811 010f 0203  ................
 000863e0: 0405 0607 0809 0a0b 0c0d 0e0f 1026 0087  .............&..
 000863f0: 0800 8808 0328 036f 0355 0358 035a 0302  .....(.o.U.X.Z..
 00086400: 0341 0324 03d9 0403 dc04 03dd 0403 0103  .A.$............
 00086410: 2303 5703 5903 2903 2503 5f03 6a03 4403  #.W.Y.).%._.j.D.
 00086420: 6d03 4a03 1b03 e604 03e7 0403 d402 0380  m.J.............
@@ -113551,9 +113551,9 @@
 001bb8e0: 0806 0103 0004 0000 0117 0004 0d00 0017  ................
 001bb8f0: 0102 0100 0101 00d0 080f 0002 1700 0f01  ................
 001bb900: 0100 1701 040b 0b01 00d0 0815 1701 0001  ................
 001bb910: 0103 0000 0c3a 1700 0401 0105 0100 d008  .....:..........
 001bb920: 2014 0100 d008 2300 0400 0100 d008 270a   .....#.......'.
 001bb930: 0100 d108 0400 0117 0004 0101 0500 0100  ................
 001bb940: d208 0300 0a01 00d3 0803 0001 1401 00d3  ................
-001bb950: 080a 0004 0001 00d4 0802 907f c3c9 9764  ...............d
-001bb960: 77e3 0a24 240a                           w..$$.
+001bb950: 080a 0004 0001 00d4 0802 8162 2d2d 5d3d  ...........b--]=
+001bb960: fb45 0a24 240a                           .E.$$.
exit 1

I tried using various gcexports tools to decode the 2 files and diff them but the contents are pretty much identical. Consulting upstream Go team for more information on the 7-bytes diff.

@sluongng
Copy link
Contributor Author

Forgot to add: I also tried to compile the go_library several times in a loop

[
  go_library(name = "db_{}".format(i), ...)
  for i in (1, 10)
]

and was able to verify that the .x is reproducible when compiling within the same set of dependencies.

So the few bytes changes must come from the dependency upgrade, just unclear why.

@joeljeske
Copy link
Contributor

joeljeske commented Oct 14, 2024

@sluongng I discovered something related - it appears that the .x archive is not reproducible even if the interface of the package is constant.

The .x file should not change if I modify this file to return false instead of true, but it does.

package mypkg

func TestFunc() bool {
	return true
}

I do not think we are getting much/any benefit from separating the .a from the .x, as intended by #2461

@fmeum
Copy link
Member

fmeum commented Oct 14, 2024

@joeljeske Does this reproduce if you make the implementation more complicated? I wouldn't be surprised if the export data ensures that this function is inlined into every caller.

@joeljeske
Copy link
Contributor

Yes, the x archive still changes even when the implementation is more complex. Here is a draft PR with a script to show the issue. #4143

Does rules_go have any test case that asserts on this functionality? I'm not certain if this is a result of the rules_go implementation of extracting the pkgdef information, or something in golang native that is causing the instability.

In any case, I think this performance feature may not be working as intended, and there are potentially huge gains to be had 😄

@joeljeske
Copy link
Contributor

Also, should I move this to a new issue, as its not exactly the same issue as @sluongng? But it does feel very related.

@sluongng
Copy link
Contributor Author

Yeah, I think you are observing the effect described in golang/go#69547. IIUC, the Go team responds that .x not being stable is "fair" game. You can follow up in that thread or via Gophers' Slack #compilers channel.

I do not think we are getting much/any benefit from separating the .a from the .x

In a remote build setup, separating the 2 reduces the input size of GoCompilePkg actions significantly. .x is smaller and can be compressed with Zstd much better than the binary .a. So there is a benefit that our cache is more fine-grained and our actions are slimmer.

I do think though, that ultimately, a fix in upstream Go tooling would be very much desirable. If the exported "interface" in a package was not changed, the '.x' output should be stable. I did not find the time to create a follow-up with the Go team on this issue though. It would be nice if someone else could help with this.

@annapst
Copy link

annapst commented Oct 28, 2024

I think there is more missing reproducibility. I've built the same target on two machines configured the same way and got different .x files, while .a files were the same.

When inspecting with diffoscope following the example above, I've observed that constants (both private and public) are included in different (random?) order. I traced two differences to validation types and cardinality. There were also other differences that looked like ordering as well, but not for constants and not as easy to trace.

005da4b0: 6564 0133 084f 7074 696f 6e61 6c01 3108 ed.3.Optional.1.
│ -005da4c0: 5265 7175 6972 6564 0132 0103 0131 0108 Required.2...1..
│ -005da4d0: 4f70 7469 6f6e 616c 0132 0108 5265 7175 Optional.2..Requ
│ -005da4e0: 6972 6564 0133 0108 5265 7065 6174 6564 ired.3..Repeated
│ +005da4c0: 5265 7175 6972 6564 0132 0103 0132 0108 Required.2...2..
│ +005da4d0: 5265 7175 6972 6564 0133 0108 5265 7065 Required.3..Repe
│ +005da4e0: 6174 6564 0131 0108 4f70 7469 6f6e 616c ated.1..Optional
│ 005da4f0: 0000 0001 2f67 6f6f 676c 652e 676f 6c61 ..../google.gola
│ 005da500: 6e67 2e6f 7267 2f70 726f 746f 6275 662f ng.org/protobuf/
│ 005da510: 7265 666c 6563 742f 7072 6f74 6f72 6566 reflect/protoref
│ 005da520: 6c65 6374 010e 4361 7264 696e 616c 6974 lect..Cardinalit

@fmeum
Copy link
Member

fmeum commented Oct 28, 2024

@annapst Could you file an issue with upstream for this? That's a pretty clean diff and the existing explanations for why there may be a difference don't apply.

@sluongng
Copy link
Contributor Author

with a setup like this

~/work/misc/test-go> cat importcfg
# import config
~/work/misc/test-go> cat temp.go
package lib

type mytype int

const (
    validationTypeOther mytype = iota
    validationTypeMessage
    validationTypeGroup
    validationTypeMap
    validationTypeRepeatedVarint
    validationTypeRepeatedFixed32
    validationTypeRepeatedFixed64
    validationTypeVarint
    validationTypeFixed32
    validationTypeFixed64
    validationTypeBytes
    validationTypeUTF8String
    validationTypeMessageSetItem
)
~/work/misc/test-go> cat go.mod
module github.com/foo/lib

go 1.23

I could not reproduce

~/work/misc/test-go> go tool compile -o lib.x -linkobj lib.a -p github.com/foo/lib -complete -goversion go1.23.2 -c=4 -shared -nolocalimports -importcfg importcfg -pack ./temp.go && sha256sum lib.x && sha256sum lib.a && rm -f lib*
6d9a7e512c9db5b9ffbc5cfe74bf413fa0bf730fe2cd3a6b68f80154ca8b5652  lib.x
5bd6d2693bfd2176e119ed4ecbf74175ce39f9e827588f77ca35aeabef69ed52  lib.a
~/work/misc/test-go> go tool compile -o lib.x -linkobj lib.a -p github.com/foo/lib -complete -goversion go1.23.2 -c=4 -shared -nolocalimports -importcfg importcfg -pack ./temp.go && sha256sum lib.x && sha256sum lib.a && rm -f lib*
6d9a7e512c9db5b9ffbc5cfe74bf413fa0bf730fe2cd3a6b68f80154ca8b5652  lib.x
5bd6d2693bfd2176e119ed4ecbf74175ce39f9e827588f77ca35aeabef69ed52  lib.a
~/work/misc/test-go> go tool compile -o lib.x -linkobj lib.a -p github.com/foo/lib -complete -goversion go1.23.2 -c=4 -shared -nolocalimports -importcfg importcfg -pack ./temp.go && sha256sum lib.x && sha256sum lib.a && rm -f lib*
6d9a7e512c9db5b9ffbc5cfe74bf413fa0bf730fe2cd3a6b68f80154ca8b5652  lib.x
5bd6d2693bfd2176e119ed4ecbf74175ce39f9e827588f77ca35aeabef69ed52  lib.a
~/work/misc/test-go> go tool compile -o lib.x -linkobj lib.a -p github.com/foo/lib -complete -goversion go1.23.2 -c=4 -shared -nolocalimports -importcfg importcfg -pack ./temp.go && sha256sum lib.x && sha256sum lib.a && rm -f lib*
6d9a7e512c9db5b9ffbc5cfe74bf413fa0bf730fe2cd3a6b68f80154ca8b5652  lib.x
5bd6d2693bfd2176e119ed4ecbf74175ce39f9e827588f77ca35aeabef69ed52  lib.a
~/work/misc/test-go> go tool compile -o lib.x -linkobj lib.a -p github.com/foo/lib -complete -goversion go1.23.2 -c=4 -shared -nolocalimports -importcfg importcfg -pack ./temp.go && sha256sum lib.x && sha256sum lib.a && rm -f lib*
6d9a7e512c9db5b9ffbc5cfe74bf413fa0bf730fe2cd3a6b68f80154ca8b5652  lib.x
5bd6d2693bfd2176e119ed4ecbf74175ce39f9e827588f77ca35aeabef69ed52  lib.a
~/work/misc/test-go> go tool compile -o lib.x -linkobj lib.a -p github.com/foo/lib -complete -goversion go1.23.2 -c=4 -shared -nolocalimports -importcfg importcfg -pack ./temp.go && sha256sum lib.x && sha256sum lib.a && rm -f lib*
6d9a7e512c9db5b9ffbc5cfe74bf413fa0bf730fe2cd3a6b68f80154ca8b5652  lib.x
5bd6d2693bfd2176e119ed4ecbf74175ce39f9e827588f77ca35aeabef69ed52  lib.a

It would be nice to get a small reproduction from your end @annapst
And yeah, if it can be reproduced, the issue would need to be filed upstream to Go's compiler team

@annapst
Copy link

annapst commented Nov 9, 2024

I also didn't reproduce it starting from a small example. After digging into differences between our repository build and a simple go example, I found that the difference in ".x" files on the same commit appears only when I run builds with nogo, so I'm not sure if this is go compiler problem or not. I'll be going after upgrading rules_go to latest and checking if that's still the case, I see nogo got separated into a separate action, will share an update then!

@fmeum
Copy link
Member

fmeum commented Nov 10, 2024

I'm actually able to reproduce this in rules_go's own tests now and will investigate further.

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

No branches or pull requests

4 participants