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

cmd/compile: (more) bad pcln entries when compiling type parametric functions #49523

Closed
aarzilli opened this issue Nov 11, 2021 · 6 comments
Closed
Labels
NeedsFix okay-after-beta1
Milestone

Comments

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented Nov 11, 2021

I found one more issue like #49436 so I decided to write a program to find all of them. The program is https://github.com/aarzilli/badlngenerics/.

When run with test/typeparam/*.go it produces https://gist.github.com/aarzilli/7c4ba1e87e986f0ec1a89cdec455392a as output. The output format is

append.go:29 0x454e8d main._Append

which means that after compiling append.go with -gcflags='-N -l' instruction 0x454e8d, which belongs to an instantiation of main._Append, is assigned to line 29 of append.go, which happens to be outside of the body of main._Append in the source file.

Some of the output could be false positives but I've spot-checked some of it and it looks like it's detecting real problems.
There's a few of them.

Tested with:

go version devel go1.18-c49627e81b Thu Nov 11 11:47:33 2021 +0000 linux/amd64

cc @dr2chase

@danscales
Copy link
Contributor

@danscales danscales commented Nov 13, 2021

Thanks for doing this, Alessandro! I will take this, I can see one small change that will fix a bunch of these wrong positions (which are often because of new nodes added for implicit operations like conversions). Then I'll try to hunt down the remaining wrong positions.

@danscales danscales self-assigned this Nov 13, 2021
@danscales danscales added this to the Go1.18 milestone Nov 13, 2021
@danscales danscales added the okay-after-beta1 label Nov 13, 2021
@danscales
Copy link
Contributor

@danscales danscales commented Nov 14, 2021

@aarzilli I think the "-l" option was not gettiing parsed correctly in the gcflags option, so there was still inlining happening. I changed the line to:

        out, err := exec.Command("go", "build", "-o", tgt, "-gcflags=-N", "-gcflags=-l", path).CombinedOutput()

and got fewer errors, because now there was no inlining.

Then I was able to fix the rest of the errors with the upcoming change (some still show up because of auto-generated functions, which I think is fine).

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 14, 2021

Change https://golang.org/cl/363835 mentions this issue: cmd/compile: fix position info for implicit nodes due to generics

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Nov 15, 2021

AFAICT passing multiple -gcflags options like that does a different thing, they don't get concatenated, the second one replaces the first one. You can see it with -x, with -gcflags='-N -l' the call to the compiler is:

/usr/local/go/pkg/tool/linux_amd64/compile -o $WORK/b001/_pkg_.a -trimpath "$WORK/b001=>" -p main -complete -buildid LK_v-5NKMW9Krnt-raXK/LK_v-5NKMW9Krnt-raXK -N -l -c=4 -nolocalimports -importcfg $WORK/b001/importcfg -pack ./test.go $WORK/b001/_gomod_.go

with -gcflags=-N -gcflags=-l it's:

/usr/local/go/pkg/tool/linux_amd64/compile -o $WORK/b001/_pkg_.a -trimpath "$WORK/b001=>" -p main -complete -buildid cdP0z86604nOtUfGIdfU/cdP0z86604nOtUfGIdfU -l -c=4 -nolocalimports -importcfg $WORK/b001/importcfg -pack ./test.go $WORK/b001/_gomod_.go

Enabling optimizations removes some of the instructions that had incorrect line numbers, hence fewer errors.

@cagedmantis cagedmantis added the NeedsFix label Nov 15, 2021
@danscales
Copy link
Contributor

@danscales danscales commented Nov 15, 2021

@aarzilli Yes, thanks, that makes sense now. I thought inlining was still happening, but it was a position problem related to multiple return values. I fixed that issue as well. So, there are no issues, except for auto-generated code (which I think are fine), when running bad.go now.

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Nov 16, 2021

Thanks!

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

No branches or pull requests

4 participants