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

cmd/compile: wrong location list for function argument #72053

Open
aarzilli opened this issue Mar 1, 2025 · 5 comments · May be fixed by #72805
Open

cmd/compile: wrong location list for function argument #72053

aarzilli opened this issue Mar 1, 2025 · 5 comments · May be fixed by #72805
Assignees
@dr2chase
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@aarzilli
Copy link
Contributor

aarzilli commented Mar 1, 2025

Given the following program:

package main

import (
	"fmt"
	"strings"
)

func main() {
	u := Address{Addr: "127.0.0.1"}
	fmt.Println(u) // line 10
}

type Address struct {
	TLS  bool
	Addr string
}

func (a Address) String() string {
	sb := new(strings.Builder)
	sb.WriteString(a.Addr)
	return sb.String()
}

For function main.Address.String the first entry in the location list looks like this:

DW_OP_reg0 DW_OP_piece 0x1 DW_OP_piece 0x7 DW_OP_reg3 DW_OP_piece 0x8 DW_OP_piece 0x7 DW_OP_reg2 DW_OP_piece 0x8

this is wrong, the total size of this location list is 31 bytes while the type of the variable is only 24 bytes. It looks like a spurious 7 byte empty piece is inserted in the middle of the two fields of the string type, for some reason.

Reproduces on go 1.24, 1.23 and 1.22.

Originally reported as: go-delve/delve#3923

cc @dr2chase
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 1, 2025
@aarzilli aarzilli mentioned this issue Mar 1, 2025
Open
@JunyangShao JunyangShao reopened this Mar 3, 2025
@JunyangShao
Copy link
Contributor

@golang/compiler

@JunyangShao JunyangShao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 3, 2025
@mknyszek
Copy link
Contributor

mknyszek commented Mar 5, 2025

CC @thanm maybe? :) In case you've got some interest in this with your DWARF5 work.

@mknyszek mknyszek added this to the Backlog milestone Mar 5, 2025
@derekparker
Copy link
Contributor

Mentioned this in the Delve <> Go team meeting, but I am taking a look at this and hope to send in a CL this week or the next.

derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/ssa: fix ComputePadding
04b89b4 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/ssa: fix ComputePadding
870467b 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/ssa: fix ComputePadding
98fecb1 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/abi: fix ComputePadding
854301d 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/abi: fix ComputePadding
2f200e9 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/abi: fix ComputePadding
91dc042 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
derekparker added a commit to derekparker/go that referenced this issue Mar 11, 2025
@derekparker
cmd/compile/internal/abi: fix ComputePadding
e05e2af 
Fixes the ComputePadding calculation to take into account the padding
added for the current offset. This fixes an issue where padding can be
added incorrectly for certain structs.

Related: go-delve/delve#3923

Fixes golang#72053
@derekparker derekparker linked a pull request Mar 11, 2025 that will close this issue
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/656736 mentions this issue: cmd/compile/internal/abi: fix ComputePadding

@derekparker derekparker mentioned this issue Mar 12, 2025
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/657355 mentions this issue: dwtest: add test for go#72053

gopherbot pushed a commit to golang/debug that referenced this issue Mar 15, 2025
@derekparker @gopherbot
dwtest: add test for go#72053
70c1efa 
Adds a test for https://go.dev/issue/72053
Note the fix is submitted as https://go.dev/cl/656736

For [golang/go#72053](golang/go#72053)

Change-Id: Iea247439f95cce85bf9a1560dd475be5048ec97a
GitHub-Last-Rev: dcd050f
GitHub-Pull-Request: #22
Reviewed-on: https://go-review.googlesource.com/c/debug/+/657355
Reviewed-by: David Chase <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dr2chase dr2chase

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

cmd/compile/internal/abi: fix ComputePadding derekparker/go
6 participants
@aarzilli @mknyszek @derekparker @dr2chase @gopherbot @JunyangShao