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

runtime: TestLldbPython failing with 'no intvar' #31188

Open
josharian opened this issue Apr 1, 2019 · 23 comments
Labels
Milestone

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Apr 1, 2019

During all.bash, I 100% reproducibly get:

--- FAIL: TestLldbPython (7.82s)
    runtime-lldb_test.go:187: Unexpected lldb output:
        Created target
        Created breakpoint
        Process launched
        Hit breakpoint
        Stopped at main.go:10
        no intvar
FAIL
FAIL	runtime	35.143s

On macOS.

$ lldb --version
lldb-1001.0.12.1
  Swift-5.0

The builders aren't failing because of #31123.

cc @heschik @aarzilli @thanm @dr2chase

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Apr 1, 2019

I have LLDB on my laptop and it passes.

$ go test -v -run=Lldb runtime
=== RUN   TestLldbPython
--- PASS: TestLldbPython (6.96s)
PASS
ok  	runtime	7.014s

My LLDB is older though.

$ lldb --version
lldb-1000.11.37.1
  Swift-4.2
@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 1, 2019

Fails quite nicely for me, with the matching lldb version.
Just trying to use lldb by hand, it fails horribly with some sort of Python nonsense.
Using delve, the generated binary seems fine.

I assume this has something to do with updates to macOS.
Does anyone actually use lldb with Go?
It's never been anything other than incredibly frustrating for me.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 1, 2019

Don't try this with MacPorts Python:

python
Python 2.7.16 (default, Mar  9 2019, 03:43:10) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.0 (clang-1000.11.45.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.append("/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Resources/Python")
>>> import lldb
Fatal Python error: PyThreadState_Get: no current thread
Abort trap: 6
@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Apr 1, 2019

Does anyone actually use lldb with Go?

I do, because of frustrations with gdb and codesigning. But I may be the only one.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 1, 2019

Gdb worked for me last week, but not this week. Seriously.
I even code-signed it just now, but for some reason that didn't work.
Presumably security was enhanced in some way when I upgraded to Mojave.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Apr 1, 2019

But I may be the only one.

I sometimes do as well. But my use case doesn't really involve the python part (or even nicely formatted variables, etc.).

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 1, 2019

dwarfdump -verify is very whiny.
But it's whiny back to at least go-1.11.5
I don't know if this is the problem or not.

@andybons andybons added this to the Unplanned milestone Apr 1, 2019
@josharian josharian modified the milestones: Unplanned, Go1.13 Apr 1, 2019
@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Apr 1, 2019

@andybons I’ve changed the milestone to 1.13, if for no other reason than it causes all.bash to fail. If we want to punt indefinitely, we should add a t.Skip to the test. I’ll defer to @dr2chase as to whether we want to fix soon or skip soon.

@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Apr 1, 2019

@josharian sounds good to me. No objection on my end.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 1, 2019

It's not good that it causes all.bash to fail for Mojave users.
"Fix soon" assumes that "soon" is an actual option for "fix".

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 1, 2019

Change https://golang.org/cl/170281 mentions this issue: runtime: skip broken TestLldbPython

gopherbot pushed a commit that referenced this issue Apr 1, 2019
It's broken on our builders (once we enabled dev mode on our Macs,
see CL 170339)

Updates #31188

Change-Id: Iceea65dc79576057b401a461bfe39254fed1f7ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/170281
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@siebenmann

This comment has been minimized.

Copy link

@siebenmann siebenmann commented Apr 1, 2019

This may be related to whatever is going on in #29244, which causes a somewhat different failure on some Linux machines with the right (or wrong) combination of lldb and kernel versions.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 3, 2019

It looks, though not quite proven, like this is a newish enhancement to a bug in lldb. The accompanying dwarf dumping tools fail to properly recognize "base address selection entries" (DWARF 4, section 2.6.2, page 30) and instead decodes them as beginning and ending address offsets, in turn mis-interpreting the actual beginning and ending offsets that follow.

Apparently lldb has historically failed to properly process base address selection entries and we hacked around it for Darwin, but now their mere presence causes failure. The string "base address ent" appears nowhere in the lldb sources.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Apr 3, 2019

It's reading the section start to finish rather than actually using the references? Great.

Unfortunately a base address selector is shorter than a location list entry -- it doesn't have a location expression length -- so a simple strategy of just zeroing them out won't work.

When I wrote this, I was uncomfortable actually changing the size of the .debug_loc section for fear of breaking subsequent link steps. We do have some precedent for that now, though, since DWARF compression does it. So it might be possible to actually strip the base address entries rather than leaving them around as junk data.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 3, 2019

Change https://golang.org/cl/170638 mentions this issue: cmd/link: revert CL 98075 because LLDB is very picky now

@thanm

This comment has been minimized.

Copy link
Member

@thanm thanm commented Apr 3, 2019

@heschik we're already changing DWARF section sizes in the object file reader, e.g. patchDWARFName.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Apr 3, 2019

That's changing the size of symbols on their way in to the linker, which should be pretty harmless. I'm pretty sure that removeDwarfAddrListBaseAddress used to run after sections had been laid out and relocations had been processed, so changing symbol sizes would have changed section sizes and layouts. But if that was ever true, it's not now, because https://golang.org/cl/111237 moved all this work before section layout is done. So it should be totally fine to just remove the base address entries entirely.

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 3, 2019

To be clear, I am benchmarking a CL that removes the base address entries entirely, and though that made dwarfdump happy (and gnu objdump and delve remained happy), it did not fix the lldb test failure.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 5, 2019

Change https://golang.org/cl/170798 mentions this issue: [release-branch.go1.12] runtime: skip broken TestLldbPython

gopherbot pushed a commit that referenced this issue Apr 5, 2019
It's broken on our builders (once we enabled dev mode on our Macs,
see CL 170339)

Updates #31188

Change-Id: Iceea65dc79576057b401a461bfe39254fed1f7ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/170281
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 46c3e21)
Reviewed-on: https://go-review.googlesource.com/c/go/+/170798
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 5, 2019

On Mojave, it looks like your option for debugging Go is Delve, even with the Go binaries fixed to make dwarfdump happy. The binaries on Macos were in fact technically incorrect because there were no references to the BAS entries in .debug_loc. Objdump (from macports, gobjdump) would diagnose this problem correctly, dwarfdump would get completely confused.

Gdb from homebrew or from macports does not work, even with updated codesigning, even with with a properly configured .gdbinit (must contain set startup-with-shell off), even with proper python support (port install gdb +python27).

@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented Apr 5, 2019

Also, just to be clear, I'm going to work on other problems. I cannot see recommending anything but Delve to Go users on Macos, because so far attempting to use the other debuggers merely wastes time.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 9, 2019

Change https://golang.org/cl/171158 mentions this issue: cmd/link: replace Base Address Selection Entry w/ new relocation

gopherbot pushed a commit that referenced this issue Apr 23, 2019
This was originally

Revert "cmd/link: fix up debug_range for dsymutil (revert CL 72371)"

which has the effect of no longer using Base Address Selection
Entries in DWARF.  However, the build-time costs of that are
about 2%, so instead the hacky fixup that generated technically
incorrect DWARF was removed from the linker, and the choice
is instead made in the compiler, dependent on platform, but
also under control of a flag so that we can report this bug
against LLDB/dsymutil/dwarfdump (really, the LLVM dwarf
libraries).

This however does not solve #31188; debugging still fails,
but dwarfdump no longer complains.  There are at least two
LLDB bugs involved, and this change will at allow us
to report them without them being rejected because our
now-obsolete workaround for the first bug creates
not-quite-DWARF.

Updates #31188.

Change-Id: I5300c51ad202147bab7333329ebe961623d2b47d
Reviewed-on: https://go-review.googlesource.com/c/go/+/170638
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@dr2chase

This comment has been minimized.

Copy link
Contributor

@dr2chase dr2chase commented May 3, 2019

By-the-way, this is looking more like someone enhanced the bejeezus out of the LLVM Dwarf library concurrent with the Mojave-related XCode upgrade ; all sorts of stuff is broken, and it's not the Go side that's broken. This hasn't been proven in this case yet, but given how the last few weeks have been spent it looks like a good bet.

Just for example, if you want to debug with lldb, you'll need to turn off compressed dwarf, because the splitdwarf trick no longer works.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.