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: untyped types reaching backend #23834

Closed
mdempsky opened this issue Feb 14, 2018 · 24 comments

Comments

Projects
None yet
5 participants
@mdempsky
Copy link
Member

commented Feb 14, 2018

I created CL 94027, which validates that the SSA backend never sees untyped types. This should be safe, because the compiler frontend should eliminate all untyped types during type checking.

However, the CL doesn't currently work, suggesting the frontend is doing something (at least technically) wrong.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2018

Fixing this might be tricky, but an easy way to help would be to find some minimal test cases that reproduce failures.

  1. Apply CL 94027. (Maybe change the two f.Fatalf calls to f.Logf, so you still have a working compiler.)

  2. Run "go install cmd/compile" and then "go build -a std cmd".

  3. Look in the output for lines like:

     ../src/strconv/atoi.go:160:15: internal compiler error: allocating value with type untyped bool
    
  4. Figure out how to reproduce that error with as little code as possible. For example, this is a pretty minimal repro case (maybe it can be further simplified):

     package p
    
     func syntaxError(... interface{}) error
    
     func ParseUint(s string) (uint64, error) {
             if len(s) == 0 {
                     return 0, syntaxError("", s)
             }
             return 0, nil
     }
    
@namusyaka

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

@mdempsky I don't know if I can solve this, but since I am interested, I'll try to investigate it.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2018

@namusyaka That would be great, thanks! Feel free to ask here if you have any questions.

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

The example above is caused by the generated .eq.[2]interface{} method.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2018

@fraenkel Thanks! So just

package p
type t [2]interface{}

repros that issue.

Further steps for folks interested in helping debug/fix this:

  1. Run "go tool compile -W repro.go" and look for "untyped bool". (Make sure you're sync'd past c3e8da6, or else this will be printed as just "bool".)

  2. The output is a bit tricky to understand if you're not familiar with the compiler's AST representation (see cmd/compile/internal/gc/syntax.go for hints), but you should be able to spot things like:

    .   FOR l(1) colas(true) tc(1) ARRAY-[2]interface {}
    .   .   LT l(1) tc(1) untyped bool
    .   .   .   NAME-p..autotmp_4 a(true) l(1) x(0) class(PAUTO) esc(N) tc(1) assigned used int
    .   .   .   NAME-p..autotmp_5 a(true) l(1) x(0) class(PAUTO) esc(N) tc(1) assigned used int
    

    or

    .   .   IF l(1) tc(1)
    .   .   .   OROR l(1) tc(1) hascall untyped bool
    .   .   .   .   NE l(1) tc(1) untyped bool
    .   .   .   .   .   ITAB l(1) tc(1) PTR64-*uintptr
    

    These two cases reveal that during typechecking we're not forcing if or for conditions to a concrete type.

  3. Find the appropriate places to add x = defaultlit(x, nil). This will probably be right after the corresponding x = typecheck(x, Erv). For example, in typecheck1's case OIF, I'd try putting it right after the n.Left = typecheck(n.Left, erv).

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

Fixed the OIF and OFOR. Do you want separate CLs?
The next one(s) identified are:
. . . . . ANDAND l(6) tc(1) hascall untyped bool
. . . . . . ANDAND l(6) tc(1) hascall untyped bool
. . . . . . . NE l(6) tc(1) hascall untyped bool
. . . . . . . EQ l(6) tc(1) hascall untyped bool
. . . . . . CONVNOP l(6) tc(1) hascall untyped bool
. . . . . . . LITERAL-true a(true) l(6) tc(1) untyped bool

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

Separate CLs is great. Thanks for working on this!

@gopherbot

This comment has been minimized.

Copy link

commented Feb 15, 2018

Change https://golang.org/cl/94475 mentions this issue: cmd/compile: Convert untyped bool for OIF and OFOR

@gopherbot

This comment has been minimized.

Copy link

commented Feb 15, 2018

Change https://golang.org/cl/94495 mentions this issue: cmd/compile: Convert untyped bool for comparison operators

gopherbot pushed a commit that referenced this issue Feb 15, 2018

cmd/compile: convert untyped bool for OIF and OFOR
Updates #23834.

Change-Id: I92aca9108590a0c7de774f4fad7ded97105e3cb8
Reviewed-on: https://go-review.googlesource.com/94475
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Feb 25, 2018

Change https://golang.org/cl/97016 mentions this issue: cmd/compile: convert untyped bool for OEQ AND OCALLFUNC in certain case

@gopherbot

This comment has been minimized.

Copy link

commented Feb 25, 2018

Change https://golang.org/cl/97035 mentions this issue: cmd/compile: avoid untyped bool when comparison

@gopherbot

This comment has been minimized.

Copy link

commented Feb 25, 2018

Change https://golang.org/cl/97001 mentions this issue: cmd/compile: convert untyped bool during walkCases

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

With these last 3 CLs, there are no more reported issues.

I had changed the original CL to issue Warnl instead of Fatalf so I could see all the issues.

gopherbot pushed a commit that referenced this issue Feb 26, 2018

cmd/compile: fix typechecking in finishcompare
Previously, finishcompare just used SetTypecheck, but this didn't
recursively update any untyped bool typed subexpressions. This CL
changes it to call typecheck, which correctly handles this.

Also cleaned up outdated code for simplifying logic.

Updates #23834

Change-Id: Ic7f92d2a77c2eb74024ee97815205371761c1c90
Reviewed-on: https://go-review.googlesource.com/97035
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

gopherbot pushed a commit that referenced this issue Feb 27, 2018

cmd/compile: convert untyped bool during walkCases
Updates #23834.

Change-Id: I1789525a992d37aae9e9b69c1e9d91437d3d0d3b
Reviewed-on: https://go-review.googlesource.com/97001
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

gopherbot pushed a commit that referenced this issue Feb 28, 2018

cmd/compile: remove duplicates by using finishcompare
Updates #23834

Change-Id: If05001f9fd6b97d72069f440102eec6e371908dd
Reviewed-on: https://go-review.googlesource.com/97016
Run-TryBot: Kunpei Sakai <namusyaka@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Mar 1, 2018

Change https://golang.org/cl/97856 mentions this issue: cmd/compile: convert untyped bool during various walks

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

A bit stumped on how to fix the "last" issue I can see.
They are all showing up as:
autogenerated:1: allocating value with type untyped bool

But I cannot seem to easily identify what the generated code is.

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

I know where the last fix needs to go and I have an ugly hack to make it all work but just seems wrong.
After the very last finishcompare in walkcompare, we need to fix up the types. The problem is that its conditional because of the OCONVNOP that is injected. It also needs to use defaultlit2.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

But I cannot seem to easily identify what the generated code is.

You can try adding ssa.Func's Name field to the logging output. It's probably code generated by alg.go (which makes me think the issue is the ORANGE->OFOR loop lowering that I mentioned in CL 97856).

I know where the last fix needs to go and I have an ugly hack to make it all work but just seems wrong.

If you're able to identify minimized test cases, I can probably help identify where the missing defaultlits are.

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2018

Its the eq func for pipe using the following:

type atomicError struct{ v interface{} }

type pipe struct {
rerr atomicError
werr atomicError
}

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2018

Ohh, I think the issue is OCONVNOP doesn't do type propagation like I thought it does.

If you change finishcompare to just:

func finishcompare(n, r *Node, init *Nodes) *Node {
	r = typecheck(r, Erv)
	r = walkexpr(r, init)
	r = conv(r, n.Type)
	return r
}

it looks to handle that test case. (As an actual fix, I'd suggest adding a convnop helper function that does the same thing as conv, but makes sure typecheck turns the newly introduced OCONV into OCONVNOP or OLITERAL.)

Does that handle all remaining cases?

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2018

Well, that just ends badly....

<autogenerated>:1: cannot convert type..eq."".scase(<node INDREGSP> = .autotmp_3, <node INDREGSP> = .autotmp_4) (type bool) to type untyped bool
<autogenerated>:1: cannot convert !type..eq."".mcentral(<node INDREGSP> = .autotmp_6, <node INDREGSP> = .autotmp_7) (type bool) to type untyped bool
<autogenerated>:1: cannot convert !runtime.memequal(<node INDREGSP> = .autotmp_6, <node INDREGSP> = .autotmp_7, <node INDREGSP> = 40) (type bool) to type untyped bool
@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2018

Added a defaultlit and all looks better.

gopherbot pushed a commit that referenced this issue Mar 2, 2018

cmd/compile: convert type during finishcompare
When recursively calling walkexpr, r.Type is still the untyped value.
It then sometimes recursively calls finishcompare, which complains that
you can't compare the resulting expression to that untyped value.

Updates #23834.

Change-Id: I6b7acd3970ceaff8da9216bfa0ae24aca5dee828
Reviewed-on: https://go-review.googlesource.com/97856
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Mar 2, 2018

Change https://golang.org/cl/98295 mentions this issue: cmd/compile: convert untyped bool during walkrange

@namusyaka

This comment has been minimized.

Copy link
Member

commented Mar 2, 2018

My last CL will get rid of untyped bools in all sample codes.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 3, 2018

Change https://golang.org/cl/98337 mentions this issue: cmd/compile: prevent untyped types from reaching walk

@gopherbot gopherbot closed this in a3b3284 Mar 7, 2018

@golang golang locked and limited conversation to collaborators Mar 7, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.