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
Closed

cmd/compile: untyped types reaching backend #23834

mdempsky opened this issue Feb 14, 2018 · 24 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@mdempsky
Copy link
Member

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 mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2018
@mdempsky
Copy link
Member Author

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
Copy link
Member

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

@mdempsky
Copy link
Member Author

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

@fraenkel
Copy link
Contributor

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

@mdempsky
Copy link
Member Author

mdempsky 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
Copy link
Contributor

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
Copy link
Contributor

Separate CLs is great. Thanks for working on this!

@gopherbot
Copy link

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

@gopherbot
Copy link

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
Updates #23834.

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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@fraenkel
Copy link
Contributor

fraenkel 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
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
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
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
Copy link

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

@fraenkel
Copy link
Contributor

fraenkel 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
Copy link
Contributor

fraenkel 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
Copy link
Member Author

mdempsky 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
Copy link
Contributor

fraenkel 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
Copy link
Member Author

mdempsky 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
Copy link
Contributor

fraenkel 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
Copy link
Contributor

fraenkel commented Mar 2, 2018

Added a defaultlit and all looks better.

gopherbot pushed a commit that referenced this issue Mar 2, 2018
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
Copy link

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

@namusyaka
Copy link
Member

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

@gopherbot
Copy link

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

@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.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants