Repository: google/pprof
Pull request #891 (2024-09-01 15:43:33)

Use llvm-symbolizer's JSON output to provide function start lines
When analyzing a perf.data profile converted automatically via perf_to_profile via pprof -raw perf.data, no function start lines (s=0) are present in any of the locations. With https://github.com/google/pprof/commit/813a5fbdbec8a66f7a5aedb876e1b2c3ee0f99ac, this can be easily solved by using the same JSON frame data from llvm-symbolizer to provide StartLine for Function.start_line. This solves https://github.com/google/pprof/issues/823.

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/google/pprof/pull/891?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) Report
Attention: Patch coverage is `14.28571%` with `6 lines` in your changes missing coverage. Please review.
> Project coverage is 64.30%. Comparing base [(`0ed6a68`)](https://app.codecov.io/gh/google/pprof/commit/0ed6a68c8d9e6d5df69a1f2694c7b5c3297fdf4f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) to head [(`659b4d1`)](https://app.codecov.io/gh/google/pprof/commit/659b4d175d23e50a7148fa33dc56511932150ab8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).
> Report is 36 commits behind head on main.

| [Files with missing lines](https://app.codecov.io/gh/google/pprof/pull/891?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) | Patch % | Lines |
|---|---|---|
| [internal/binutils/addr2liner\_llvm.go](https://app.codecov.io/gh/google/pprof/pull/891?src=pr&el=tree&filepath=internal%2Fbinutils%2Faddr2liner_llvm.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-aW50ZXJuYWwvYmludXRpbHMvYWRkcjJsaW5lcl9sbHZtLmdv) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/google/pprof/pull/891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   66.86%   64.30%   -2.57%     
==========================================
  Files          44       44              
  Lines        9824     9827       +3     
==========================================
- Hits         6569     6319     -250     
- Misses       2794     3081     +287     
+ Partials      461      427      -34     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/google/pprof/pull/891?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).

aalexand: A few CI tests failed:

```
++ go test github.com/google/pprof/fuzz github.com/google/pprof/internal/binutils github.com/google/pprof/internal/driver github.com/google/pprof/internal/elfexec github.com/google/pprof/internal/graph github.com/google/pprof/internal/measurement github.com/google/pprof/internal/report github.com/google/pprof/internal/symbolizer github.com/google/pprof/internal/symbolz github.com/google/pprof/profile
ok  	github.com/google/pprof/fuzz	0.003s
--- FAIL: TestObjFile (0.80s)
    --- FAIL: TestObjFile/fixed_load_address (0.67s)
        binutils_test.go:379: SourceLine for main: got [{main /tmp/hello.c 3 0 0}]; want [{main /tmp/hello.c 3 0 3}]
    --- FAIL: TestObjFile/simulated_ASLR_address (0.13s)
        binutils_test.go:379: SourceLine for main: got [{main /tmp/hello.c 3 0 0}]; want [{main /tmp/hello.c 3 0 3}]
FAIL
FAIL	github.com/google/pprof/internal/binutils	0.860s
```
insilications: Fixed TestPEFile windows test that were failing and were obviously wrong. I have no idea why TestObjFile linux tests were failing in the CI tests. Let's try again please?
aalexand: Looks like the CI still failed.
insilications: > Maybe the CI setup needs to install llvm-symbolizer.

Could be... The TestPEFile windows tests were fixed because `ci / test-windows` is not failing anymore. I have no idea why it is failing on Linux. In my Linux system I even added a temporary debug log to double check: `t.Logf("DEBUG SourceLine for main: got %v; want %v\n", gotFrames, wantFrames)`:

```
=== RUN   TestObjFile
=== RUN   TestObjFile/fixed_load_address
    binutils_test.go:378: DEBUG SourceLine for main: got [{main /tmp/hello.c 3 0 3}]; want [{main /tmp/hello.c 3 0 3}]
=== RUN   TestObjFile/simulated_ASLR_address
    binutils_test.go:378: DEBUG SourceLine for main: got [{main /tmp/hello.c 3 0 3}]; want [{main /tmp/hello.c 3 0 3}]
--- PASS: TestObjFile (0.10s)
    --- PASS: TestObjFile/fixed_load_address (0.09s)
    --- PASS: TestObjFile/simulated_ASLR_address (0.01s)
```
    

I wish we could solve this because it would close https://github.com/google/pprof/issues/823
aalexand: > Could be... The TestPEFile windows tests were fixed because `ci / test-windows` is not failing anymore. I have no idea why it is failing on Linux. In my Linux system I even added a temporary debug log to double check: `t.Logf("DEBUG SourceLine for main: got %v; want %v\n", gotFrames, wantFrames)`:

[binutils.go](https://github.com/google/pprof/blob/main/internal/binutils/binutils.go) falls back to nm or binutils' addr2line if llvm-symbolizer is not available. I think this is what might be happening since perhaps the CI environment doesn't have llvm-symbolizer. I suspect we need to figure out how to install llvm-symbolizer around this line: https://github.com/google/pprof/blob/a0b0bb1d4134f37be44de6c328680339b0fc13ad/.github/workflows/ci.yaml#L149
insilications: > [binutils.go](https://github.com/google/pprof/blob/main/internal/binutils/binutils.go) falls back to nm or binutils' addr2line if llvm-symbolizer is not available. I think this is what might be happening since perhaps the CI environment doesn't have llvm-symbolizer. I suspect we need to figure out how to install llvm-symbolizer around this line:
> 
> https://github.com/google/pprof/blob/a0b0bb1d4134f37be44de6c328680339b0fc13ad/.github/workflows/ci.yaml#L149

I have pushed changes to `ci.yaml` to attempt to fix it. Lets check if it passes now?

insilications: I ran the CI workflow on my fork. It is now passing all `test-linux` jobs on `ubuntu-22.04`. But it's failing on `ubuntu-20.04`. My added `Verify llvm-symbolizer installation` step shows that `llvm-symbolizer --version` on `ubuntu-22.04` is:

```
llvm-symbolizer
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: znver3
```

But on `ubuntu-20.04` is:

```
LLVM (http://llvm.org/):
  LLVM version 10.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: (unknown)
```
insilications: Last commit addresses the issue of an outdated `llvm-symbolizer` on Ubuntu 20.04. It is now passing all tests.
aalexand: Sorry it's taking me a while to review this. I've been staring at the code on and off, I'm just not a fan of how much this complicates the CI configuration with this code to get llvm/clang installed. But I guess there is no better way so I should just bite the bullet and LGTM.
insilications: > Sorry it's taking me a while to review this. I've been staring at the code on and off, I'm just not a fan of how much this complicates the CI configuration with this code to get llvm/clang installed. But I guess there is no better way so I should just bite the bullet and LGTM.

Not a problem at all. Take the time you need to give it a thorough review.

Yeah, the additional complexity is sad but it is required... Ubuntu 20.04 provides LLVM 10's `llvm-symbolizer`, which is very old and does not produce a JSON data frame with `StartLine`. The alternative would be to add a minimum version check (LLVM 14) before the test that uses `llvm-symbolizer` and skip it: https://github.com/google/pprof/blob/a0b0bb1d4134f37be44de6c328680339b0fc13ad/internal/binutils/binutils_test.go#L336

Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 61ab05a..25890e3 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -144,6 +144,23 @@ jobs:
         run: |
           google-chrome --version
 
+      - name: Add LLVM 14.0 repository to ensure llvm-symbolizer 14.0.0+ on Ubuntu 20.04
+        if: matrix.os == 'ubuntu-20.04'
+        run: |
+          wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
+          sudo add-apt-repository "deb http://apt.llvm.org/focal/ llvm-toolchain-focal-14 main"
+          sudo apt-get update
+
+      - name: Install llvm-symbolizer
+        run: |
+          if [ "${{ matrix.os }}" = "ubuntu-20.04" ]; then
+            sudo apt-get install -y llvm-14 clang-14
+            sudo update-alternatives --install /usr/bin/llvm-symbolizer llvm-symbolizer /usr/bin/llvm-symbolizer-14 100
+          else
+            sudo apt-get update
+            sudo apt-get install -y llvm clang
+          fi
+
       - name: Fetch dependencies
         run: |
           sudo apt-get install graphviz
@@ -154,6 +171,10 @@ jobs:
           # Add PATH for installed tools.
           echo "$GOPATH/bin:$PATH" >> $GITHUB_PATH
 
+      - name: Check llvm-symbolizer installation
+        run: |
+          llvm-symbolizer --version
+
       - name: Run the script
         run: |
           go version
diff --git a/driver/driver.go b/driver/driver.go
index 6cbf669..989aac3 100644
--- a/driver/driver.go
+++ b/driver/driver.go
@@ -186,10 +186,11 @@ type ObjFile interface {
 
 // A Frame describes a single line in a source file.
 type Frame struct {
-	Func   string // name of function
-	File   string // source file name
-	Line   int    // line in file
-	Column int    // column in file
+	Func      string // name of function
+	File      string // source file name
+	Line      int    // line in file
+	Column    int    // column in file
+	StartLine int    // start line of function (if available)
 }
 
 // A Sym describes a single symbol in an object file.
diff --git a/internal/binutils/addr2liner_llvm.go b/internal/binutils/addr2liner_llvm.go
index 5e51644..2f5d97e 100644
--- a/internal/binutils/addr2liner_llvm.go
+++ b/internal/binutils/addr2liner_llvm.go
@@ -151,10 +151,11 @@ func (d *llvmSymbolizer) readCodeFrames() ([]plugin.Frame, error) {
 		Address    string `json:"Address"`
 		ModuleName string `json:"ModuleName"`
 		Symbol     []struct {
-			Line         int    `json:"Line"`
-			Column       int    `json:"Column"`
-			FunctionName string `json:"FunctionName"`
-			FileName     string `json:"FileName"`
+			Line          int    `json:"Line"`
+			Column        int    `json:"Column"`
+			FunctionName  string `json:"FunctionName"`
+			FileName      string `json:"FileName"`
+			StartLine     int    `json:"StartLine"`
 		} `json:"Symbol"`
 	}
 	if err := json.Unmarshal([]byte(line), &frame); err != nil {
@@ -162,7 +163,7 @@ func (d *llvmSymbolizer) readCodeFrames() ([]plugin.Frame, error) {
 	}
 	var stack []plugin.Frame
 	for _, s := range frame.Symbol {
-		stack = append(stack, plugin.Frame{Func: s.FunctionName, File: s.FileName, Line: s.Line, Column: s.Column})
+		stack = append(stack, plugin.Frame{Func: s.FunctionName, File: s.FileName, Line: s.Line, Column: s.Column, StartLine: s.StartLine})
 	}
 	return stack, nil
 }
diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go
index f2ef987..2692c72 100644
--- a/internal/plugin/plugin.go
+++ b/internal/plugin/plugin.go
@@ -159,10 +159,11 @@ type ObjFile interface {
 
 // A Frame describes a location in a single line in a source file.
 type Frame struct {
-	Func   string // name of function
-	File   string // source file name
-	Line   int    // line in file
-	Column int    // column in line (if available)
+	Func          string // name of function
+	File          string // source file name
+	Line          int    // line in file
+	Column        int    // column in line (if available)
+	StartLine     int    // start line of function (if available)
 }
 
 // A Sym describes a single symbol in an object file.
diff --git a/internal/symbolizer/symbolizer.go b/internal/symbolizer/symbolizer.go
index 70b4047..0d45136 100644
--- a/internal/symbolizer/symbolizer.go
+++ b/internal/symbolizer/symbolizer.go
@@ -228,6 +228,7 @@ func symbolizeOneMapping(m *profile.Mapping, locs []*profile.Location, obj plugi
 				Name:       frame.Func,
 				SystemName: frame.Func,
 				Filename:   frame.File,
+				StartLine:  int64(frame.StartLine),
 			})
 			l.Line[i] = profile.Line{
 				Function: f,


Test patch:
diff --git a/internal/binutils/binutils_test.go b/internal/binutils/binutils_test.go
index dd931f4..7657088 100644
--- a/internal/binutils/binutils_test.go
+++ b/internal/binutils/binutils_test.go
@@ -373,7 +373,7 @@ func TestObjFile(t *testing.T) {
 				t.Fatalf("SourceLine: unexpected error %v", err)
 			}
 			wantFrames := []plugin.Frame{
-				{Func: "main", File: "/tmp/hello.c", Line: 3},
+				{Func: "main", File: "/tmp/hello.c", Line: 3, StartLine: 3},
 			}
 			if !reflect.DeepEqual(gotFrames, wantFrames) {
 				t.Fatalf("SourceLine for main: got %v; want %v\n", gotFrames, wantFrames)
@@ -402,17 +402,17 @@ func TestMachoFiles(t *testing.T) {
 		{"normal mapping", "exe_mac_64", 0x100000000, math.MaxUint64, 0,
 			0x100000f50, "_main",
 			[]plugin.Frame{
-				{Func: "main", File: "/tmp/hello.c", Line: 3},
+				{Func: "main", File: "/tmp/hello.c", Line: 3, StartLine: 3},
 			}},
 		{"other mapping", "exe_mac_64", 0x200000000, math.MaxUint64, 0,
 			0x200000f50, "_main",
 			[]plugin.Frame{
-				{Func: "main", File: "/tmp/hello.c", Line: 3},
+				{Func: "main", File: "/tmp/hello.c", Line: 3, StartLine: 3},
 			}},
 		{"lib normal mapping", "lib_mac_64", 0, math.MaxUint64, 0,
 			0xfa0, "_bar",
 			[]plugin.Frame{
-				{Func: "bar", File: "/tmp/lib.c", Line: 5},
+				{Func: "bar", File: "/tmp/lib.c", Line: 5, StartLine: 5},
 			}},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -461,8 +461,8 @@ func TestLLVMSymbolizer(t *testing.T) {
 		frames []plugin.Frame
 	}{
 		{0x10, false, []plugin.Frame{
-			{Func: "Inlined_0x10", File: "foo.h", Line: 0, Column: 0},
-			{Func: "Func_0x10", File: "foo.c", Line: 2, Column: 1},
+			{Func: "Inlined_0x10", File: "foo.h", Line: 0, Column: 0, StartLine: 0},
+			{Func: "Func_0x10", File: "foo.c", Line: 2, Column: 1, StartLine: 2},
 		}},
 		{0x20, true, []plugin.Frame{
 			{Func: "foo_0x20", File: "0x20 8"},
@@ -532,7 +532,7 @@ func TestPEFile(t *testing.T) {
 				t.Fatalf("SourceLine: unexpected error %v", err)
 			}
 			wantFrames := []plugin.Frame{
-				{Func: "main", File: "hello.c", Line: 3, Column: 12},
+				{Func: "main", File: "hello.c", Line: 3, Column: 12, StartLine: 3},
 			}
 			if !reflect.DeepEqual(gotFrames, wantFrames) {
 				t.Fatalf("SourceLine for main: got %v; want %v\n", gotFrames, wantFrames)
diff --git a/internal/binutils/testdata/fake-llvm-symbolizer b/internal/binutils/testdata/fake-llvm-symbolizer
index 507761c..491a2cd 100755
--- a/internal/binutils/testdata/fake-llvm-symbolizer
+++ b/internal/binutils/testdata/fake-llvm-symbolizer
@@ -29,7 +29,7 @@ while read line; do
   addr=$3
   case ${kind} in
   CODE)
-    echo "{\"Address\":\"${addr}\",\"ModuleName\":\"${fname}\",\"Symbol\":[{\"Column\":0,\"FileName\":\"${fname}.h\",\"FunctionName\":\"Inlined_${addr}\",\"Line\":0},{\"Column\":1,\"FileName\":\"${fname}.c\",\"FunctionName\":\"Func_${addr}\",\"Line\":2}]}"
+    echo "{\"Address\":\"${addr}\",\"ModuleName\":\"${fname}\",\"Symbol\":[{\"Column\":0,\"FileName\":\"${fname}.h\",\"FunctionName\":\"Inlined_${addr}\",\"Line\":0,\"StartLine\":0},{\"Column\":1,\"FileName\":\"${fname}.c\",\"FunctionName\":\"Func_${addr}\",\"Line\":2,\"StartLine\":2}]}"
     ;;
   DATA)
     echo "{\"Address\":\"${addr}\",\"ModuleName\":\"${fname}\",\"Data\":{\"Name\":\"${fname}_${addr}\",\"Size\":\"0x8\",\"Start\":\"${addr}\"}}"