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

Add support for deduplicating type descriptors by relocating code to point at main module's types #62

Closed
wants to merge 10 commits into from

Conversation

eh-steve
Copy link

@eh-steve eh-steve commented Sep 22, 2022

Previously, compiled code which used type assertions or switches wouldn't always work as expected, since duplicate *_types would be present in both the main module and the object file, and type assertion uses pointer equality to determine whether types match (and the object code would reference the local types)

This PR relocates code which references identical type descriptors (according to their compiler generated type hash being identical and type definitions being identical according to runtime.typesEqual()) to point at the main module's *_types:

Example

func HandleBytes(input interface{}) {
    bytesData := input.([]byte)
}

might previously fail with interface conversion: interface {} is []uint8, not []uint8 (types from different scopes), even if the incoming input is a []byte.

Instead, in these cases you had to use reflection to (safely) extract the underlying concrete type from the interface, e.g.:

func HandleBytes(input interface{}) {
    rval := reflect.ValueOf(input)
	if rval.Type().Kind() == reflect.Slice && rval.Type().Elem().Kind() == reflect.Uint8 {
        bytesData := rval.Bytes()
    }
}

This PR fixed the broken behaviour so type assertion and type switches on duplicated types works as expected

Anonymous added 6 commits September 29, 2022 14:12
… assist in debugging + fix cutab layout to correctly offset when creating multi-object, multi-package linkers - previously stack traces had incorrect files due to incorrect cutab + filetab
@pkujhd
Copy link
Owner

pkujhd commented Oct 8, 2022

@eh-steve, thanks for your pr, but some changes cannot adapt to the lower golang version, i will review it and merge it manually.

@pkujhd pkujhd added the enhancement New feature or request label Oct 8, 2022
@eh-steve
Copy link
Author

eh-steve commented Oct 8, 2022

I'm happy to port the pclntab changes to the lower versions - I might be making some more changes once I get to the bottom of some other bugs

@eh-steve eh-steve mentioned this pull request Oct 17, 2022
3 tasks
@pkujhd
Copy link
Owner

pkujhd commented Oct 26, 2022

@eh-steve

When the loader adds a symbol (ld.go:AddSymbol), if the symbol already exists in loader, the loader will be use the symbol in loader, it will be avoid to duplicate type symbol, but it will be lead to far address for R_ADDR on windows platform.
As the same time, i will deal it with issue #61 and this pr.

@pkujhd pkujhd closed this Oct 26, 2022
@pkujhd
Copy link
Owner

pkujhd commented Oct 26, 2022

I will merge bug fix manually, thanks for your pr

@eh-steve
Copy link
Author

eh-steve commented Oct 28, 2022

Hi, your fixes don't actually cover everything that has been fixed in this PR - would you be up for discussing these changes further?

I might add some tests which demonstrate each fix

@pkujhd
Copy link
Owner

pkujhd commented Oct 31, 2022

@eh-steve
Some bugs I don't find testcases.
Now, I am finding golang source code by import path and ASM function warpper, if you have testcases, please give me an issue or some code, thanks.

@eh-steve
Copy link
Author

eh-steve commented Nov 2, 2022

Hi @pkujhd,

I've opened a chunky PR which hopefully demonstrates the purpose of each commit (and adds a new JIT compiler package). The jit tests should be able to be run for each commit on the branch.

I would suggest we try to fix all the outstanding goloader bugs in that branch, then attempt to backport the changes for older Go versions rather than picking individual commits into master which aren't 100% working.

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

Successfully merging this pull request may close these issues.

2 participants