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

Added support for parsing multiple filename tables #218

Closed
wants to merge 3 commits into from
Closed

Added support for parsing multiple filename tables #218

wants to merge 3 commits into from

Conversation

Omie
Copy link
Contributor

@Omie Omie commented Aug 28, 2015

refs #143 #215 (may be #79 )

  • added a type DebugLines that can hold number of DebugLineInfo
  • added a supporting attribute to DebugLineInfo called 'Lookup' which is to be
    used to quickly lookup if file exists in FileNames slice
  • added supporting methods to lookup and return corresponding DebugLineInfo
  • changed the debug_line parsing behavior to read all the available tables and
    push them to DebugLines
  • since Process.lineInfo is now a slice, it was breaking AllPCsBetween as well
  • updated that function's definition to accept a new filename parameter to be
    able to extract related DebugLineInfo
  • updated calls to AllPCsBetween
  • fixed tests that were broken due to attribute type change in Process
  • updated _fixtures/cgotest program to include stdio.h, so that it updates
    .debug_line header
  • added a test to check 'next' in a cgo binary

- added a type DebugLines that can hold number of DebugLineInfo
- added a supporting attribute to DebugLineInfo called 'Lookup' which is to be
used to quickly lookup if file exists in FileNames slice
- added supporting methods to lookup and return corresponding DebugLineInfo
- changed the debug_line parsing behavior to read all the available tables and
push them to DebugLines

- since Process.lineInfo is now a slice, it was breaking AllPCsBetween as well
- updated that function's definition to accept a new filename parameter to be
able to extract related DebugLineInfo
- updated calls to AllPCsBetween

- fixed tests that were broken due to attribute type change in Process
- updated _fixtures/cgotest program to include stdio.h, so that it updates
.debug_line header
- added a test to check 'next' in a cgo binary
- OSX - 1.4 does not support cgo, handle that in new testcase
@Omie
Copy link
Contributor Author

Omie commented Aug 28, 2015

Someone has to see whats going wrong with osx-go1.4.2, I don't have Macbook :-(

@@ -495,6 +495,34 @@ func TestSwitchThread(t *testing.T) {
})
}

func TestCGONext(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this test fail without your patch? I think there is already a CGO next test (I know it was my suggestion to write this test), but I want to make sure we have a test that triggers the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This test fails without my patch with exact same error. (with updated cgotest fixture only).
Another cgo test passes (with/without extra C header).

- use line comments instead of block comments
@Omie
Copy link
Contributor Author

Omie commented Aug 29, 2015

wonder whats wrong with Linux+1.4.2 on Travis now. All tests pass fine for me on 1.4.2, 1.5.
Linux+1.4.2 passed on Travis for earlier build, cleanup commit shouldn't break anything as I only altered comments and a blank line there.

@derekparker
Copy link
Member

Merged, thanks!

I squashed the commits and applied the following cleanup:

diff --git a/dwarf/line/line_parser.go b/dwarf/line/line_parser.go
index da07468..def5a11 100644
--- a/dwarf/line/line_parser.go
+++ b/dwarf/line/line_parser.go
@@ -37,28 +37,22 @@ type FileEntry struct {
 type DebugLines []*DebugLineInfo

 func (d *DebugLines) GetLineInfo(name string) *DebugLineInfo {
-   //Find in which table file exists.
-   //Return that table
+   // Find in which table file exists and return it.
    for _, l := range *d {
-       if fe := l.GetFileEntry(name); fe != nil {
+       if _, ok := l.Lookup[name]; ok {
            return l
        }
    }
-
    return nil
 }

-func (d *DebugLineInfo) GetFileEntry(name string) *FileEntry {
-   return d.Lookup[name]
-}
-
 func Parse(data []byte) DebugLines {
    var (
        lines = make(DebugLines, 0)
        buf   = bytes.NewBuffer(data)
    )

-   // we have to scan multiple file name tables here
+   // We have to parse multiple file name tables here.
    for buf.Len() > 0 {
        dbl := new(DebugLineInfo)
        dbl.Lookup = make(map[string]*FileEntry)
@@ -67,10 +61,10 @@ func Parse(data []byte) DebugLines {
        parseIncludeDirs(dbl, buf)
        parseFileEntries(dbl, buf)

-       //Instructions size calculation breakdown:
-       //- dbl.Prologue.UnitLength is the length of the entire unit, not including the 4 bytes to represent that length.
-       //- dbl.Prologue.Length is the length of the prologue not including unit length, version or the prologue length itself.
-       //- So you have UnitLength - PrologueLength - (version_length_bytes(2) + prologue_length_bytes(4)).
+       // Instructions size calculation breakdown:
+       //   - dbl.Prologue.UnitLength is the length of the entire unit, not including the 4 bytes to represent that length.
+       //   - dbl.Prologue.Length is the length of the prologue not including unit length, version or prologue length itself.
+       //   - So you have UnitLength - PrologueLength - (version_length_bytes(2) + prologue_length_bytes(4)).
        dbl.Instructions = buf.Next(int(dbl.Prologue.UnitLength - dbl.Prologue.Length - 6))

        lines = append(lines, dbl)
diff --git a/dwarf/line/state_machine.go b/dwarf/line/state_machine.go
index 6cc1c8b..b9255f8 100644
--- a/dwarf/line/state_machine.go
+++ b/dwarf/line/state_machine.go
@@ -75,13 +75,12 @@ func newStateMachine(dbl *DebugLineInfo) *StateMachine {
 // Returns all PCs for a given file/line. Useful for loops where the 'for' line
 // could be split amongst 2 PCs.
 func (dbl *DebugLines) AllPCsForFileLine(f string, l int) (pcs []uint64) {
-   lineInfo := dbl.GetLineInfo(f)
-
    var (
-       sm        = newStateMachine(lineInfo)
-       buf       = bytes.NewBuffer(lineInfo.Instructions)
        foundFile bool
        lastAddr  uint64
+       lineInfo  = dbl.GetLineInfo(f)
+       sm        = newStateMachine(lineInfo)
+       buf       = bytes.NewBuffer(lineInfo.Instructions)
    )

    for b, err := buf.ReadByte(); err == nil; b, err = buf.ReadByte() {
diff --git a/proc/proc_test.go b/proc/proc_test.go
index 5e91b31..c685fbe 100644
--- a/proc/proc_test.go
+++ b/proc/proc_test.go
@@ -496,8 +496,7 @@ func TestSwitchThread(t *testing.T) {
 }

 func TestCGONext(t *testing.T) {
-   //test if one can do 'next' in a cgo binary
-
+   // Test if one can do 'next' in a cgo binary
    // 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants