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.

Sign up for GitHub

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

Jump to bottom

Fix a few dwarf64 parsing problems. Nothing fancy. #27

Open
wants to merge 2 commits into
base: sparc64
Choose a base branch
from
Open

Fix a few dwarf64 parsing problems. Nothing fancy. #27

wants to merge 2 commits into from

Conversation

quenelle
Copy link

This fixes bugs that show up when dwarf from Studio compilers
gets mixed in via crt files.

Fix a few dwarf64 parsing problems. Nothing fancy.
059806e 
This fixes bugs that show up when dwarf from Studio compilers
gets mixed in via crt files.
@quenelle quenelle mentioned this pull request Jul 19, 2017
Open
@quenelle
Copy link
Author

More specifically:
Fixes #25

binarycrusader
binarycrusader requested changes Jul 21, 2017
View reviewed changes
src/debug/dwarf/open.go Outdated
x, y := d.info[4], d.info[5]
if (d.info[0] == 255 && d.info[1] == 255) {
// 64-bit DWARF has 4 bytes of FF, then 8 byte size,
// then 2 byte version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting seems a bit off here; consider running 'gofmt -w' on this file.

src/debug/dwarf/unit.go Outdated
aoff = b.uint32()
header_bytes_read += 4
} else {
// For now ignore the high bits
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/now/now,/

src/debug/dwarf/unit.go Outdated
@@ -62,12 +62,23 @@ func (d *Data) parseUnits() ([]unit, error) {
var n Offset
n, u.is64 = b.unitLength()
vers := b.uint16()
var header_bytes_read uint
header_bytes_read = 2 // Not counting size
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 'hbytes' instead of 'header_bytes_read' or 'hlen' ?

Fix a few dwarf64 parsing problems.
9c4c1b2 
This fixes bugs that show up when dwarf from Studio compilers
gets mixed in via crt files.
binarycrusader
binarycrusader reviewed Jul 21, 2017
View reviewed changes
Copy link
Collaborator

@binarycrusader binarycrusader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all of the Go tests pass still after this change (e.g. ./all.bash)?

aclements
aclements reviewed Aug 2, 2017
View reviewed changes
Copy link
Contributor

@aclements aclements left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me. Just a few non-semantic changes.

This would be great to upstream separate of the rest of the sparc changes. I'm really surprised this hasn't bitten us before.

src/debug/dwarf/unit.go
hbcount += 4
} else {
// For now, ignore the high bits
aoff = uint32(b.uint64())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend checking for overflow here. See the equivalent code in Data.parseTypes.

src/debug/dwarf/unit.go
@@ -62,12 +62,24 @@ func (d *Data) parseUnits() ([]unit, error) {
var n Offset
n, u.is64 = b.unitLength()
vers := b.uint16()
// Count the bytes of header read, not counting size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than counting bytes, I'd suggest following what parseTypes does. Here, just say "hdroff := b.off", then below say "b.bytes(int(n - (b.off - hdroff)))".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had actually suggested the same thing to Chris offline :-)

@4ad
Copy link
Owner

4ad commented Aug 2, 2017

This would be great to upstream separate of the rest of the sparc changes. I'm really surprised this hasn't bitten us before.

I agree.

@quenelle
Copy link
Author

quenelle commented Aug 3, 2017

Sorry for the delay here.
I'm on medical leave after breaking my hip and wrist at the same time.
It'll be some time before I'm reasonably functional at a keyboard again.
If someone else is interested in finishing this, I would not object.

Changes needed are:

  1. Instead of counting header bytes as they are read in, just read b.off
    to see how many more to skip. As it is in typeunit.go/parseTypes()

  2. Check for overflow of the abbrev table offset in unit.go/New as it's
    done in typeunit.go/parseTypes

  3. add a test case. First try using a simple hello world binary from
    Studio cc in -m64 mode. If that does fail, then grab the
    frankenstein binary from the current failing test on machines
    with Studio dwarf in the crt files. You can check a sample elf
    binary (minimal) into src/debug/dwarf/testdata/one

dw64studiocrt.zip

I've attached a binary that should show the error if you use DWARF() to open it.

@quenelle
Copy link
Author

quenelle commented Aug 3, 2017

I agree pushing this separate from the sparc64 stuff seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aclements aclements aclements left review comments

@binarycrusader binarycrusader binarycrusader requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@quenelle @4ad @binarycrusader @aclements