Picking offset of g struct inside TLS correctly #177
Conversation
|
PS. ocfaode in dwarf/op/op.go seemed like a variable rename gone wrong, I've taken the liberty of renaming it to opcode. |
|
This looks great. If you can port your code over for directly reading from TLS as was discussed in https://groups.google.com/d/msgid/delve-dev/55AFE938.5000300%40gmail.com I'll verify and merge as soon as I get back to my machine on Saturday. |
aarzilli
force-pushed
the
cgo-tls-offset-bug
branch
from
419aea3
to
8b78a52
Compare
|
the patch doesn't work _fixtures/testnextnethttp has the runtime.iscgo flag set but is internally linked so the offset comes out -16, runtime.iscgo is not the right thing to check |
aarzilli
force-pushed
the
cgo-tls-offset-bug
branch
from
8b78a52
to
bd13e32
Compare
|
I can't test the second commit on OS X. |
|
Just tried to verify on OSX, ran into some compilation issues, fixed them, and then got a panic when running the tests. Here's the diff: And heres the panic: I can investigate tomorrow if you don't have access to an OSX machine. |
|
That panic seems unrelated to the OS X-specific change. Will this line: |
|
Yes it is possible. The reason is because on OSX the key in that map is the threads' mach port, which is distinct from the PID. It is preferable to use Here is my current diff: And here is the error I get when running tests: |
|
Some problem with reader.(*Reader).NextCompileUnit on externally linked executables? Weird. |
|
PS. isn't this the same thing as Issue #79? |
|
Yes it is, was just going to link to it, hah. It looks like with this patch you're already checking for external linkage with Does this patch work on Go 1.5beta2? I haven't tested yet, and won't have time to for a few hours. If it does work, we can just ignore the issue for now and fix it in a separate patch, so we don't continue piling onto this one. |
Yes, but the code that computes isextld is the one that's failing, because it needs dwarf info, so it doesn't help.
I haven't tested it, I do not know what the state of delve is on 1.5. |
|
Right, good point, should have thought that through more before posting, trying to juggle too many things at the same time. Let me test this out a bit more, and try to think through a solution to this linkage issue on OSX. |
| hardwareBreakpointUsage: make([]bool, 4), | ||
| } | ||
| } | ||
|
|
||
| func (a *AMD64) SetCurGInstructions(ver GoVersion, isextld bool) { |
There was a problem hiding this comment.
Rename to SetGStructOffset.
|
I'd like to merge this in, regardless of Go1.5 issues on OSX (golang/go#11887) and CGO/Go1.4.2 issues on OSX (https://github.com/derekparker/delve/issues/79) as I believe these are separate concerns. Aside from the comments above, can you apply the following patch to this branch. The only major change is (for now) ignoring the CGO test you added on OSX for Go on Darwin with version < 1.5. diff --git a/proc/proc.go b/proc/proc.go
index 61748f4..04b0905 100644
--- a/proc/proc.go
+++ b/proc/proc.go
@@ -683,9 +683,7 @@ func (dbp *Process) execPtraceFunc(fn func()) {
}
func (dbp *Process) getGoInformation() (ver GoVersion, isextld bool, err error) {
- th := dbp.Threads[dbp.Pid]
-
- vv, err := th.EvalPackageVariable("runtime.buildVersion")
+ vv, err := dbp.CurrentThread.EvalPackageVariable("runtime.buildVersion")
if err != nil {
err = fmt.Errorf("Could not determine version number: %v\n", err)
return
@@ -709,6 +707,5 @@ func (dbp *Process) getGoInformation() (ver GoVersion, isextld bool, err error)
break
}
}
-
return
}
diff --git a/proc/proc_test.go b/proc/proc_test.go
index 8e09ca0..145bcd2 100644
--- a/proc/proc_test.go
+++ b/proc/proc_test.go
@@ -9,6 +9,7 @@ import (
"os"
"path/filepath"
"runtime"
+ "strings"
"testing"
"time"
@@ -617,6 +618,11 @@ func TestGetG(t *testing.T) {
testGSupportFunc("nocgo", t, p, fixture)
})
+ // On OSX with Go < 1.5 CGO is not supported due to: https://github.com/golang/go/issues/8973
+ if runtime.GOOS == "darwin" && strings.Contains(runtime.Version(), "1.4") {
+ return
+ }
+
withTestProcess("cgotest", t, func(p *Process, fixture protest.Fixture) {
testGSupportFunc("cgo", t, p, fixture)
})
diff --git a/proc/threads_darwin.c b/proc/threads_darwin.c
index b02febd..a0988b6 100644
--- a/proc/threads_darwin.c
+++ b/proc/threads_darwin.c
@@ -53,7 +53,7 @@ get_registers(mach_port_name_t task, x86_thread_state64_t *state) {
kern_return_t
get_identity(mach_port_name_t task, thread_identifier_info_data_t *idinfo) {
mach_msg_type_number_t idinfoCount = THREAD_IDENTIFIER_INFO_COUNT;
- return thread_info(task, THREAD_IDENTITY_INFO, (thread_info_t)idinfo, &idinfoCount);
+ return thread_info(task, THREAD_IDENTIFIER_INFO, (thread_info_t)idinfo, &idinfoCount);
}
kern_return_t
diff --git a/proc/threads_darwin.h b/proc/threads_darwin.h
index 93310f7..156c410 100644
--- a/proc/threads_darwin.h
+++ b/proc/threads_darwin.h
@@ -27,3 +27,6 @@ resume_thread(thread_act_t);
kern_return_t
set_registers(mach_port_name_t, x86_thread_state64_t*);
+
+kern_return_t
+get_identity(mach_port_name_t, thread_identifier_info_data_t *); |
… the value of runtime.buildVersion and presence of compile units created by GNU AS, instead of being fixed to -16
…d of modifying the executable code
aarzilli
force-pushed
the
cgo-tls-offset-bug
branch
from
941b295
to
0933a68
Compare
* UPdated conn confs * UPdated conn confs
The change to proc.(*Thread).executeStackProgram is necessary because it seems that cgo programs start at an address that FDEForPC doesn't know about.