pkujhd / goloader

load and run golang code at runtime.
Apache License 2.0
497 stars 58 forks source link

JIT compiler + bugfixes #66

Open eh-steve opened 1 year ago

eh-steve commented 1 year ago

This is mostly a rebased commit history of #62, plus the addition of a new jit package to allow compiling and loading Go files/packages/text, and recursively resolving their dependencies.

This allows users to build and load arbitrary Go files with any number of imports automatically.

You can simply do:


loadable, err = jit.BuildGoFiles(jit.BuildConfig{}, "./path/to/file1.go", "/path/to/file2.go")
// or
loadable, err = jit.BuildGoPackage(jit.BuildConfig{}, "./path/to/package")
// or
loadable, err = jit.BuildGoText(jit.BuildConfig{}, `
package mypackage

import "encoding/json"

func MyFunc(input []byte) (interface{}, error) {
    var output interface{}
    err := json.Unmarshal(input, &output)
    return output, err
}

`)

if err != nil {
    panic(err)
}

module, symbols, err = loadable.Load()

if err != nil {
    panic(err)
}

MyFunc := symbols["MyFunc"].(func([]byte) (interface{}, error))
result, err := MyFunc([]byte(`{"k":"v"}`))

The jit package and tests are early on in the commit history to demonstrate the purpose of each subsequent commit in fixing specific tests.

So far, testing has only been on linux/amd64, linux/arm64, darwin/amd64, darwin/arm64 (Apple M1 silicon), and windows/amd64 . Once the last few goloader bugs are ironed out, I will try to backport these changes for earlier Go versions where possible.

Versions:

OS/Arch amd64/+CGo arm64/+CGo amd64/-CGo arm64/-CGo
Linux/go-1.19.4 :heavy_check_mark: :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
Darwin/go-1.19.4 :heavy_check_mark: :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
Windows/go-1.19.4 :heavy_check_mark: :interrobang: :heavy_check_mark: :interrobang:
Linux/go-1.18.8 :heavy_check_mark: :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
Darwin/go-1.18.8 :heavy_check_mark: :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
Windows/go-1.18.8 :x: :interrobang: :heavy_check_mark: :interrobang:
Linux/go-1.17.13 :x: :x: :x: :x:
Darwin/go-1.17.13 :x: :x: :x: :x:
Windows/go-1.17.13 :x: :x: :x: :x:
Linux/go-1.16.15 :x: :x: :x: :x:
Darwin/go-1.16.15 :x: :x: :x: :x:
Windows/go-1.16.15 :x: :x: :x: :x:
Linux/go-1.15.15 :x: :x: :x: :x:
Darwin/go-1.15.15 :x: :x: :x: :x:
Windows/go-1.15.15 :x: :x: :x: :x:
Linux/go-1.14.15 :x: :x: :x: :x:
Darwin/go-1.14.15 :x: :x: :x: :x:
Windows/go-1.14.15 :x: :x: :x: :x:
Linux/go-1.13.15 :x: :x: :x: :x:
Darwin/go-1.13.15 :x: :x: :x: :x:
Windows/go-1.13.15 :x: :x: :x: :x:
Linux/go-1.12.17 :x: :x: :x: :x:
Darwin/go-1.12.17 :x: :x: :x: :x:
Windows/go-1.12.17 :x: :x: :x: :x:
Linux/go-1.11.13 :x: :x: :x: :x:
Darwin/go-1.11.13 :x: :x: :x: :x:
Windows/go-1.11.13 :x: :x: :x: :x:
Linux/go-1.10.8 :x: :x: :x: :x:
Darwin/go-1.10.8 :x: :x: :x: :x:
Windows/go-1.10.8 :x: :x: :x: :x:
Linux/go-1.9.7 :x: :x: :x: :x:
Darwin/go-1.9.7 :x: :x: :x: :x:
Windows/go-1.9.7 :x: :x: :x: :x:
pkujhd commented 1 year ago

@eh-steve , thanks. I will try to recurrence bugs and fix.

By the way, goloader does not test with cgo and asm.

eh-steve commented 1 year ago

By the way, goloader does not test with cgo and asm.

I appreciate that - my main motivation was getting common packages like net/http and math to build successfully (which directly or indirectly use CGo/asm), and my fixes do seem to work (as shown in the tests).

pkujhd commented 1 year ago

@eh-steve I try use your jit branch to run go test in jit dir on linux/amd64 , every test outputs same errors, e.g "jit_test.go:42: still have 17 unresolved external symbols despite building and linking dependencies..."

Could you give me a simple case for TestStackSplit and TestJitPanicRecoveryStackTrace this issue mentioned? Or tell me how to run your test.

Thx

eh-steve commented 1 year ago

What version of Go are you using, and which symbols remain unresolved? (I'm using 1.19.3 and it's working fine)

Can you try

go test -c
./jit.test -test.v -test.run TestStackSplit

./jit.test -test.v -test.run TestJitPanicRecovery

I've made certain core packages forbidden for re-inclusion in the JIT compiler to prevent duplication of certain atomic, reflect or runtime packages, are the missing symbols from these forbidden packages?

If so, they just need to be included in the test binary by declaring some global variables which use them.

Also I recently rebased and force pushed so you might wanna delete and re-checkout if you pulled before the rebase.

EDIT:

I just tried running via go test and got the same problem as you. It seems that when running tests directly via go test rather than through a pre-built binary, these symbols don't get included into the main binary, whereas they do if you build the test binary using -c:

        internal/cpu.X86
        internal/poll.runtime_isPollServerDescriptor
        internal/poll.runtime_pollWaitCanceled
        reflect..inittask
        reflect.bytesType
        reflect.dummy
        runtime..inittask
        runtime.defaultGOROOT
        runtime.vdsoGettimeofdaySym
        runtime.writeBarrier
        runtime.x86HasSSE41
        runtime.zerobase
        syscall.cgocaller
        syscall.runtime_AfterExec
        syscall.runtime_BeforeExec
        syscall.runtime_doAllThreadsSyscall
        time.modTimer

I could fix this by forcing the symbols to be included by importing more things in jit_test.go if you want? In the meantime it should work with -c

eh-steve commented 1 year ago

Edit: Fixed this now

Some tests are still failing randomly (e.g. TestStackSplit and TestJitPanicRecoveryStackTrace) due to some outstanding bugs in goloader which still need to be fixed.

Specifically: Things which trigger stack copies/stack splits while a dynamically loaded function is running can cause an incomplete stack unwinding (fatal error: traceback did not unwind completely) , possibly because the g's stack pointer was incorrectly incremented twice somewhere, so the unwinding doesn't ever reach the stack start. Unclear whether this is a TLS relocation issue or something else causing the sp to be incremented by the same frame size twice when calling dynamic code?

Example:

=== RUN   TestJitPanicRecoveryStackTrace/BuildGoFiles
runtime: g808: frame.sp=0xc004befd60 top=0xc004beffe0
  stack=[0xc004bec000-0xc004bf0000] n=4 max=2147483647
fatal error: traceback did not unwind completely

runtime stack:
runtime.throw({0x849c62?, 0xc00024e000?})
  /usr/local/go/src/runtime/panic.go:1047 +0x5d fp=0x7fb3f6ffca88 sp=0x7fb3f6ffca58 pc=0x439e7d
runtime.gentraceback(0x7fb40c71400b?, 0xb94c30?, 0xc00003ef00?, 0x7fb3f6ffce60?, 0x0, 0x0, 0x7fffffff, 0x7fb3f6ffce48, 0x1?, 0x0)
  /usr/local/go/src/runtime/traceback.go:561 +0x1385 fp=0x7fb3f6ffcdf8 sp=0x7fb3f6ffca88 pc=0x460d85
runtime.addOneOpenDeferFrame.func1()
  /usr/local/go/src/runtime/panic.go:645 +0x6b fp=0x7fb3f6ffce70 sp=0x7fb3f6ffcdf8 pc=0x43900b
runtime.systemstack()
  /usr/local/go/src/runtime/asm_amd64.s:492 +0x49 fp=0x7fb3f6ffce78 sp=0x7fb3f6ffce70 pc=0x46c269

goroutine 808 [running]:
runtime.systemstack_switch()
  /usr/local/go/src/runtime/asm_amd64.s:459 fp=0xc004befb20 sp=0xc004befb18 pc=0x46c200
runtime.addOneOpenDeferFrame(0x41c3e6?, 0xbac840?, 0xc004befbe8?)
  /usr/local/go/src/runtime/panic.go:644 +0x69 fp=0xc004befb60 sp=0xc004befb20 pc=0x438f49
panic({0x7a6480, 0x8dd510})
  /usr/local/go/src/runtime/panic.go:844 +0x112 fp=0xc004befc20 sp=0xc004befb60 pc=0x439792
command-line-arguments.(*SomeType).callSite4(0xc00027bd20?, {{0x7a6480, 0x8dd510}, 0x0, {0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0})
  /tmp/file1_1093370123/file4.go:16 +0x25 fp=0xc004befc40 sp=0xc004befc20 pc=0x417352ad
command-line-arguments.(*SomeType).callSite3(0x4e2e73746e656d75?, {{0x7a6480, 0x8dd510}, 0x0, {0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0})
  /tmp/file1_1093370123/file3.go:13 +0x50 fp=0xc004befca0 sp=0xc004befc40 pc=0x41735268
command-line-arguments.(*SomeType).callSite2(0xc000028034?, {{0x7a6480, 0x8dd510}, 0x0, {0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0})
  /tmp/file1_1093370123/file2.go:11 +0x50 fp=0xc004befd00 sp=0xc004befca0 pc=0x417351f8
command-line-arguments.(*SomeType).callSite1(0x0?, {{0x7a6480, 0x8dd510}, 0x0, {0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0})
  /tmp/file1_1093370123/file1.go:7 +0x50 fp=0xc004befd60 sp=0xc004befd00 pc=0x41735188
created by testing.(*T).Run
  /usr/local/go/src/testing/testing.go:1493 +0x35f
eh-steve commented 1 year ago

@pkujhd All tests are passing on my machine now, I think it's ready for review

pkujhd commented 1 year ago

Edit: Fixed this now

Some tests are still failing randomly (e.g. TestStackSplit and TestJitPanicRecoveryStackTrace) due to some outstanding bugs in goloader which still need to be fixed.

how to fixed it? I don't find the code change for goloader.

pkujhd commented 1 year ago

@eh-steve

For commit https://github.com/pkujhd/goloader/pull/66/commits/31f8f2bd0af2c793c20cae1e47c9507343b52b25, the pclntable does not split for adapter golang version < 1.16

For commit 3cd5a08, plugin use deduplicate function to deal it, but in goloader it will be lead to far address relocate error on R_ADDROFF & R_METHODOFF. By modifying read-only memory. used first module type descriptor. it could avoid it, but I worried it will be forbidden on MacOSX. I think maybe the best way are use deduplicate function to deal it and copy type descriptor with far address relocated from the first module into data segment. (it maybe means a lots of workload, because a far address type descriptor maybe is an interface)

By the way, I cannot merge pr because the current version cannot pass all tests. I will wait for the newer commit.

pkujhd commented 1 year ago

I could fix this by forcing the symbols to be included by importing more things in jit_test.go if you want? In the meantime it should work with -c

it is worked for go test -c, I run it ok on Linux, but on MacOSX get a coredump,

[signal SIGBUS: bus error code=0x2 addr=0x18f864a pc=0x18f864a]

runtime stack:
runtime.throw({0x1451678?, 0xc0000fb380?})
    /Users/pkujhd/programs/go/src/runtime/panic.go:1047 +0x5d fp=0x700004a84e98 sp=0x700004a84e68 pc=0x10390dd
runtime.sigpanic()
    /Users/pkujhd/programs/go/src/runtime/signal_unix.go:819 +0x369 fp=0x700004a84ee8 sp=0x700004a84e98 pc=0x104fb69

goroutine 9 [syscall]:
runtime.cgocall(0x18f864a, 0xc000525e38)
    /Users/pkujhd/programs/go/src/runtime/cgocall.go:158 +0x5c fp=0xc000525e10 sp=0xc000525dd8 pc=0x100573c
command-line-arguments._Cfunc_mul(0x2, 0x3)
    _cgo_gotypes.go:58 +0x4c fp=0xc000525e38 sp=0xc000525e10 pc=0x18f70e4
command-line-arguments.CGoCall(0x13c1b20?, 0x0?)
    /var/folders/_r/x5c6p7hj7pl8c4n84tp1tj240000gn/T/test_3415849336/test.go:16 +0x28 fp=0xc000525e68 sp=0xc000525e38 pc=0x18f7028
github.com/pkujhd/goloader/jit_test.TestJitCGoCall.func1(0xc0000fb1e0)
    /Users/pkujhd/Code/go/goloader/jhd/jit/jit/jit_test.go:247 +0xce fp=0xc000525f70 sp=0xc000525e68 pc=0x1377f2e
testing.tRunner(0xc0000fb1e0, 0xc000001180)
    /Users/pkujhd/programs/go/src/testing/testing.go:1446 +0x10b fp=0xc000525fc0 sp=0xc000525f70 pc=0x11122eb
testing.(*T).Run.func1()
    /Users/pkujhd/programs/go/src/testing/testing.go:1493 +0x2a fp=0xc000525fe0 sp=0xc000525fc0 pc=0x111318a
runtime.goexit()
    /Users/pkujhd/programs/go/src/runtime/asm_amd64.s:1594 +0x1 fp=0xc000525fe8 sp=0xc000525fe0 pc=0x106de01
created by testing.(*T).Run
    /Users/pkujhd/programs/go/src/testing/testing.go:1493 +0x35f
eh-steve commented 1 year ago

how to fixed it? I don't find the code change for goloader.

Ah sorry, I rebased and force pushed my branch so it got hidden in the other commits.

The problem was here in initLinker(). The runtime uses zero value offsets of funcnametab and pctab entries to mean something special (zero offset funcname means empty string, and zero offset pctab entry means an external non-Go function which we can't find stack information for).

The logic inside runtime/traceback.go (which is used in lots of places for stack copies etc) will be broken if a function has a zero offset for its nameoff or a zero offset for it's pcsp entry.

To fix it, I added an 8 byte padding to both of those tables so that valid entries never have an offset of 0.

func initLinker() *Linker {
    linker := &Linker{
        // Pad these tabs out so offsets don't start at 0, which is often used in runtime as a special value for "missing"
        // e.g. runtime/traceback.go and runtime/symtab.go both contain checks like:
        // if f.pcsp == 0 ...
        // and
        // if f.nameoff == 0
        funcnametab:  make([]byte, PtrSize),
        pctab:        make([]byte, PtrSize),
eh-steve commented 1 year ago

Hi @pkujhd, just to give you a progress update:

I've fixed a few more bugs (that already existed before), and added extra tests for them, e.g.:

and I've added a few more new features e.g.:

JIT tests are now passing on:

It's currently not possible to run on

Still TODO:

eh-steve commented 1 year ago

For commit 3cd5a08, plugin use deduplicate function to deal it, but in goloader it will be lead to far address relocate error on R_ADDROFF & R_METHODOFF. By modifying read-only memory. used first module type descriptor. it could avoid it, but I worried it will be forbidden on MacOSX. I think maybe the best way are use deduplicate function to deal it and copy type descriptor with far address relocated from the first module into data segment. (it maybe means a lots of workload, because a far address type descriptor maybe is an interface)

I don't think we can copy a type descriptor into the dynamic module without causing type assertions to fail (since they depend on pointer equality), so I think the only option which supports both type assertions and calling deadcode-eliminated methods is to patch the firstmodule's methods by modifying read-only memory.

I think I've addressed this in a fairly robust way (if a little insane!) in the file macho_darwin.go

pkujhd commented 1 year ago

@eh-steve thanks for your contribute.

About per-linker string container with its own lifecycle. it should not be released when the module unload, because maybe some string already set into the map/array container in firstmodule, when next code access it, the process will be panic.

eh-steve commented 1 year ago

About per-linker string container with its own lifecycle. it should not be released when the module unload, because maybe some string already set into the map/array container in firstmodule, when next code access it, the process will be panic.

Yeah that’s why there’s a separate method to unload the string container, and also why I added the heap string option (which is much nicer to use)

eh-steve commented 1 year ago

I've now added basic native ELF relocs for arm64 and amd64, and that seems to make the CGo test work, I'll try to add the macho equivalent shortly.

pkujhd commented 1 year ago

Yeah that’s why there’s a separate method to unload the string container, and also why I added the heap string option (which is much nicer to use)

I means the process after unload hot-fixed module, the process original code access the string lead to panic, because the module code call some function in firstmodule set the string in map/array container. so the loader must not release string container.

if you can set the string in heap to let gc manage the memory, it is better.

eh-steve commented 1 year ago

I've now added basic native ELF relocs for arm64 and amd64, and that seems to make the CGo test work, I'll try to add the macho equivalent shortly.

Mach-O now added too (tested on both x86 and apple M1 silicon).

eh-steve commented 1 year ago

@pkujhd You might also be interested in this bugfix: https://github.com/pkujhd/goloader/pull/66/commits/b42e18025d406e7b279f1f11adf19ca43900fb05

Previously it was rare, but possible to be pre-empted by the scheduler (or a signal handler) while executing JIT instructions appended to the end of the code segment due to a far address relocation. In these cases, the runtime could throw a fatal error runtime: invalid pc-encoded table because the PC addresses don't correspond to any known functions based on findfuncbuckets, and they would be missing pcvalues for things like PCSP, PCLine, PCFile, and PCDATA_UnsafePoint. This wasn't safe.

I switched to pre-emptively growing the size of any function text which had a potentially overflowing relocation, and reserved an "epilogue" for each relocation to use if needed. Then I patched all the pcvalue tables in order to reflect the correct value for this epilogue PC segment (based on the pcvalue at the relocation site). This seems to have resolved these pre-emption bugs.

eh-steve commented 1 year ago

@pkujhd I think after https://github.com/pkujhd/goloader/pull/66/commits/a5d1e361e761cbd00c4f86b747bc032ef1bf5f3c, https://github.com/pkujhd/goloader/pull/66/commits/a9f5e9414c5d51dd69f843ece154770a3b7eb019 and https://github.com/pkujhd/goloader/pull/66/commits/b42e18025d406e7b279f1f11adf19ca43900fb05 I've finally fixed the last of the latent goloader bugs which were causing intermittent test failures (2 were arm64-specific and https://github.com/pkujhd/goloader/pull/66/commits/b42e18025d406e7b279f1f11adf19ca43900fb05 was a problem for all architectures).

(I've been running this bash script on linux/arm64, linux/amd64, darwin/amd64 and darwin/arm64 (except with CGo enabled on darwin/arm64) for hours now with no failures, where previously it would fail or fault within a few minutes)

CGO_ENABLED=0 go test -c -o ./jit.test.nocgo .
CGO_ENABLED=1 go test -c -o ./jit.test .

while CGO_ENABLED=1 ./jit.test  && CGO_ENABLED=0 ./jit.test.nocgo
do
    echo "All tests passed, trying again until failure..."
done

There is one outstanding (very rare and intermittent) bug in ConvertTypesAcrossModules() on all architectures where the Elem() of a pointer *rtype would return nil somehow, resulting in this stack trace:

--- FAIL: TestConvertOldAndNewTypes (1.00s)
    --- FAIL: TestConvertOldAndNewTypes/BuildGoPackage (0.30s)
        jit_test.go:1079: unexpected panic (this is a bug): runtime error: invalid memory address or nil pointer dereference
             stack trace: goroutine 2610 [running]:
            runtime/debug.Stack()
                /opt/homebrew/Cellar/go/1.19.2/libexec/src/runtime/debug/stack.go:24 +0x64
            github.com/pkujhd/goloader.ConvertTypesAcrossModules.func1()
                /Users/user/git/goloader/convert.go:26 +0x3c
            panic({0x1009acb60, 0x100c55510})
                /opt/homebrew/Cellar/go/1.19.2/libexec/src/runtime/panic.go:884 +0x204
            github.com/pkujhd/goloader/reflectlite/reflectlite1%2e19.(*rtype).Kind(...)
                /Users/user/git/goloader/reflectlite/reflectlite1.19/type.go:1065
            github.com/pkujhd/goloader/reflectlite/reflectlite1%2e19.Value.Elem({0x129ab0610?, 0x140001a8a08?, 0x100991e20?})
                /Users/user/git/goloader/reflectlite/reflectlite1.19/value.go:226 +0x74
            github.com/pkujhd/goloader.cvt(0x140002ba000, 0x140002ba0f0, {{0x1009a9f80, 0x140001a8a08, 0x1b5}}, {0x12ac0acf0, 0x1009a9f80}, 0x0, 0x14000089680?, 0x14000072320?)
                /Users/user/git/goloader/convert.go:365 +0x9bc
            github.com/pkujhd/goloader.cvt(0x140002ba000, 0x140002ba0f0, {{0x129ab1160, 0x140001a8000, 0x199}}, {0x12ac0acf0, 0x129fc9160}, 0x14004c24000, 0x2?, 0x1?)
                /Users/user/git/goloader/convert.go:387 +0x82c
            github.com/pkujhd/goloader.cvt(0x140002ba000, 0x140002ba0f0, {{0x129ab10c8, 0x140001a8000, 0x16}}, {0x12ac0acf0, 0x129fc90c8}, 0x0, 0x1005f54d0?, 0x1?)
                /Users/user/git/goloader/convert.go:379 +0x6b8
            github.com/pkujhd/goloader.ConvertTypesAcrossModules(0x140001a8000?, 0x140002ba0f0, {0x129ab10c8, 0x140001a8000}, {0x129fc90c8, 0x140001a8c00})
                /Users/user/git/goloader/convert.go:51 +0x2d0
            github.com/pkujhd/goloader/jit_test.TestConvertOldAndNewTypes.func1(0x140041fe9c0)
                /Users/user/git/goloader/jit/jit_test.go:1077 +0x7d8
            testing.tRunner(0x140041fe9c0, 0x140000320c0)
                /opt/homebrew/Cellar/go/1.19.2/libexec/src/testing/testing.go:1446 +0x10c
            created by testing.(*T).Run
                /opt/homebrew/Cellar/go/1.19.2/libexec/src/testing/testing.go:1493 +0x300

But it seems to be so rare and difficult to reproduce that I'm going to ignore it for now.

eh-steve commented 1 year ago

@pkujhd FYI today I started to port some of the missing mprotect and mmap functionality for Windows in this branch https://github.com/eh-steve/goloader/commit/079d833490608d4ce1883081609b84c712d73874

It’s not finished and so doesn’t work properly yet, but just a heads up in case you were interested

eh-steve commented 1 year ago

@pkujhd FYI today I started to port some of the missing mprotect and mmap functionality for Windows in this branch eh-steve@079d833

It’s not finished and so doesn’t work properly yet, but just a heads up in case you were interested

@pkujhd I finished this off today so most of the JIT tests pass on Windows now (and it solves previous problematic method relocations overflowing due to the distance being >32 bit), but proper CGo support on Windows is still missing, and I'm unlikely to invest time in adding it (because 🪟 💩).

Also, the TestJitHttpGet test currently fails on Windows with all goroutines are asleep - deadlock which I haven't investigated properly.

I'm also unlikely to invest much more time in backporting changes to older versions of Go... Is this something you feel is critical for the project?

pkujhd commented 1 year ago

I'm also unlikely to invest much more time in backporting changes to older versions of Go... Is this something you feel is critical for the project?

because some related online projects still use golang 1.16 even golang 1.11.

eh-steve commented 1 year ago

I'm also unlikely to invest much more time in backporting changes to older versions of Go... Is this something you feel is critical for the project?

because some related online projects still use golang 1.16 even golang 1.11.

Ok, what do you think about tagging the 0.0.17 release as the last version supporting Go < 1.18, and directing people to use that version if they need older versions of Go?

Especially in light of the Go team only supporting 2 major versions at a time

pkujhd commented 1 year ago

@eh-steve I will take the time to merge this branch into the trunk and try to adapt to the lower versions of Golang, I think we should separate a new repos from the part of code(package jit) that provides compilation obj file, which is of a tool. I will not consider ending the support of the lower version of golang, because there are still many bugs in the current version. Consider maintaining a new repos when golang 2.0

eh-steve commented 1 year ago

I wish you would not reimplement parts of my commits manually as it makes it very difficult to rebase and resolve conflicts, unless you plan to repeat the implementation of the entire PR by hand?

If this is the case, I might just hard fork and maintain a separate repo...

I don't mind splitting the JIT package into a separate repo, but the tests there have exposed maybe ~20 or so bugs in goloader and provides a nice test base when porting for new Go versions or architectures.

pkujhd commented 1 year ago

@eh-steve I suggest you have a hard fork if you want make it work on golang version two major versions and with cgo.

I don't have enough to read all golang's linker,so I worry more complex mechanism to lead more unknown question.

online project use it on linux/amd64 and mmap on <4G address, avoid rewrite machine code as far as possible

In addition, thanks for fixing various bugs.

eh-steve commented 1 year ago

If I backported my changes to support the older Go versions (+1.20), would you be happy to merge this, or would you copy individual things into master by hand, one at a time?

I appreciate that linux/amd64 mostly worked ok thanks to the ability to mmap <4G addresses (and windows/darwin and arm64 had a lot more problems with the machine code), but linux/amd64 was still susceptible to:

I can reconsider about investing time backporting for older versions if I don't have to keep rebasing on top of conflicts from changes you've pulled across, but if you don't plan to merge the PR at the end, I'll probably just fork if that's ok?

eh-steve commented 1 year ago

Also I think you force pushed stuff to master?

pkujhd commented 1 year ago

Also I think you force pushed stuff to master?

I will block master branch before merge it, and try to merge it and split package. but i will take some time.

eh-steve commented 1 year ago

There's still some broken things in Go 1.20 which I'm fixing - inlinedCall struct has changed size which isn't in master yet - I might be force pushing the last commit on this branch a few times while I fix the remaining tests for 1.20.

I meant that you had force pushed onto master which means any branches off master would have conflicts, and that it's generally not a good idea to force push onto master without careful coordination with other contributors.

I think you and I have a different understanding of "merge"...

pkujhd commented 1 year ago

@eh-steve , I suggest you have a hard fork, because When I merge you jit branch and try to adapter lower golang version, I find it very difficult. I will merge your bug fixed into the trunk. Thank you again for your contribution.

eh-steve commented 1 year ago

Yeah I hard forked in https://github.com/pkujhd/goloader/pull/66/commits/e0dbc397477f5f6955a87a344b8539d40f188175

eh-steve commented 1 year ago

I will merge your bug fixed into the trunk.

I'd be curious how you plan to address the incorrect cuFiles and cuOffsets after the pclntab separation causing broken stack traces

pkujhd commented 1 year ago

I'd be curious how you plan to address the incorrect cuFiles and cuOffsets after the pclntab separation causing broken stack traces

I guess what you mean is the error of PCFile. There is an error in the previous version.

In the Golang<1.16 version, It leads to the function which isn't in the first .obj file has not correct offset of the fileno. In version>=1.16, the funcfile function will be panic

In the version of golang<1.16 PCFile will be rewritten by linker, In the version of>=1.16, linker uses cuOffset to deal it

I use a similar scheme to avoid mistakes, see commit https://github.com/pkujhd/goloader/commit/f7a07e8dd1eb5675f55e17f18d76660b86b9653f