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

golang 1.21 support #82

Closed
pkujhd opened this issue Jun 27, 2023 · 5 comments
Closed

golang 1.21 support #82

pkujhd opened this issue Jun 27, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@pkujhd
Copy link
Owner

pkujhd commented Jun 27, 2023

golang 1.21 already rc2, need adapter it

@pkujhd pkujhd added the enhancement New feature or request label Jun 27, 2023
@pkujhd pkujhd closed this as completed in d22598b Jul 8, 2023
@eh-steve
Copy link

eh-steve commented Jul 8, 2023

Your init implementation in master needs a depth first recursion or other graph traversal to init all dependencies in the right order and should also skip dependent tasks which have no functions. The current implementation doesn’t guarantee the correct ordering and can throw due to the lack of these checks.

Also you need to escape the package path using objabi.PathToPrefix otherwise packages like gopkg.in/yaml.v2 won’t get init’ed.

@eh-steve
Copy link

eh-steve commented Jul 9, 2023

You also need to handle (reloctype.R_CALL | reloctype.R_WEAK) and (reloctype.R_CALLARM64 | reloctype.R_WEAK) relocations properly

@pkujhd
Copy link
Owner Author

pkujhd commented Jul 15, 2023

Your init implementation in master needs a depth first recursion or other graph traversal to init all dependencies in the right order and should also skip dependent tasks which have no functions. The current implementation doesn’t guarantee the correct ordering and can throw due to the lack of these checks.

I think golang's linker just wants to reorder the initialization of all libraries according to their dependencies.
Because I initialize each library separately, there is no need to graph traversal operations, only let all dependent initialization of current library to init.

@pkujhd
Copy link
Owner Author

pkujhd commented Jul 15, 2023

You also need to handle (reloctype.R_CALL | reloctype.R_WEAK) and (reloctype.R_CALLARM64 | reloctype.R_WEAK) relocations properly

ok, thanks for your informations.
I find add R_WARK source code

@eh-steve
Copy link

eh-steve commented Jul 15, 2023

Because I initialize each library separately, there is no need to graph traversal operations, only let all dependent initialization of current library to init.

That's not true (and your implementation demonstrably fails in my tests).

R_INITORDER relocs are only an ordering edge and don't represent every dependency recursively, so if you have a JIT package A which depends on B which depends on C, you will only init B then A (instead of C then B then A) which might cause B to panic if it uses something from C which wasn't initialised (it doesn't matter if you later init C).

Also what I said about nfncs of dependencies being 0 making the runtime throw still hold.

For a reproducible example, check out my 1.21 WIP branch eh-steve#19 and try changing the init implementation and running the TestK8s test.

You can also look at cmd/link/internal/ld/inittask.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants