reflect: avoid dereferencing non-pointers in mapassign #718

hajimehoshi · 2017-11-07T17:39:36Z

EDIT: To run only the new test quickly, I did

go run ./tool.go test --run=TestReflectSetMapIndex ./tests/

hajimehoshi · 2017-11-10T02:47:51Z

ping @shurcooL @neelance

dmitshur · 2017-11-10T03:39:42Z

In order for me to approve this, I'll have to gain understanding of the context of the problem, and the change, in order to have confidence that this is the optimal fix. It's going to take me some time. I'll try to do it as soon as I can.

If you can provide more information about the investigation you've done before coming up with the fix, it'd be helpful to me. For example, have you considered an alternative fix where the "$get" is always generated, so that it's not neccessary to check whether it's undefined or not? Would that be better or worse? In what cases is it not generated, and do you know why that behavior was chosen?

Unfortunately, I don't know these things off the top of my head because I didn't write the original relevant code, so it takes me longer to review fixes.

hajimehoshi · 2017-11-10T04:40:20Z

Sorry for the lack of the explanation. My understanding is that pointer type values in GopherJS has $get member to de-ref. reflect.mapassign [1] accepts val that type is unsafe.Pointer and assumes that this is a pointer value. However, this assumption is not right when the value is, e.g., a fixed sized array. So I though we can check if val is a pointer or not just by checking $get existence. Actually, the same idiom is used at reflect.keyFor [2]. (EDIT: not mapaccess but keyFor)

[1] https://github.com/gopherjs/gopherjs/blob/master/compiler/natives/src/reflect/reflect.go#L521
[2] https://github.com/gopherjs/gopherjs/blob/master/compiler/natives/src/reflect/reflect.go#L503

I understand that GopherJS people are quite busy. Thank you for taking time.

dmitshur · 2017-11-10T05:01:28Z

tests/misc_test.go

+func TestJSONWithFixedArray(t *testing.T) {
+	var v map[string][2]float64
+	if err := json.Unmarshal([]byte(`{"a": [350, 350]}`), &v); err != nil {
+		t.Error(err)


Since the bug and the fix are in reflect package, can we make the test lower level and use reflect directly? Relying on encoding/json is not optimal, because its internal implementation details may change, rendering the test ineffective.

Done: Used reflect package directly.

hajimehoshi · 2017-11-16T04:01:02Z

ping

hajimehoshi · 2017-11-21T15:16:14Z

Found that this fix causes a new error on unmarshaling JSON, I couldn't make a minimized case though.

dmitshur · 2017-11-21T20:30:00Z

That's pretty surprising. How big is the non-minimized case? Are you able to share it?

hajimehoshi · 2017-11-22T00:55:03Z

Ah sorry, the crash was not related to this fix. Please ignore my last comment :-)

FlorianUekermann · 2017-12-01T15:17:39Z

Given that my bug report dates back to July, I'm super happy someone finally found the problem! Some comments:

The commit message titles need some context on what code is being changed. Maybe reflect: avoid dereferencing non-pointers in mapassign or something that mentions the get method instead of dereferencing.
What's up with compiler/natives/fs_vfsdata.go in both commits?
Squash or clean up the individual commits. You are adding a function in the first and deleting it in the second.
The approach of testing reflect directly seems like a good idea. But I maybe the test should check that the results are correct as well. That would add confident in the fix as well.

hajimehoshi · 2017-12-02T07:55:51Z

The commit message titles need some context on what code is being changed.

Thanks, I've changed the title.

What's up with compiler/natives/fs_vfsdata.go in both commits?

Not sure what you meant. All tests passed anyway.

Squash or clean up the individual commits.

Agreed but this is what GopherJS members would do.

But I maybe the test should check that the results are correct as well.

Well, I'll do that later.

hajimehoshi · 2017-12-02T08:13:16Z

Ugh, m.MapIndex(k) crashes:

--- FAIL: TestReflectSetMapIndex (0.04s)          
TypeError: dst.$set is not a function             
    at typedmemmove (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/reflect.go:487:3)        
    at Object.$packages.reflect.Value.ptr.MapIndex (/reflect/value.go:1075:4)                        
    at TestReflectSetMapIndex (/github.com/gopherjs/gopherjs/tests/misc_test.go:633:3)               
    at tRunner (/testing/testing.go:746:3)        
    at $goroutine (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/test.340830512:1471:19)    
    at $runScheduled (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/test.340830512:1511:7)  
    at $schedule (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/test.340830512:1527:5)      
    at $go (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/test.340830512:1503:3)            
    at Object.<anonymous> (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/test.340830512:35117:1)
    at Object.<anonymous> (/Users/hajimehoshi/go/src/github.com/gopherjs/gopherjs/test.340830512:35120:4)
    at Module._compile (module.js:573:30)         
    at Object.Module._extensions..js (module.js:584:10)                                              
    at Module.load (module.js:507:32)             
    at tryModuleLoad (module.js:470:12)           
    at Function.Module._load (module.js:462:3)    
    at Function.Module.runMain (module.js:609:10) 
    at startup (bootstrap_node.js:158:16)         
    at bootstrap_node.js:578:3                    
FAIL    github.com/gopherjs/gopherjs/tests      0.881s                                               
exit status 1

I'd want to commit this PR as it is and make another PR for the fix of MapIndex.

dmitshur · 2017-12-02T09:24:30Z

m.MapIndex(k) crashes

Is that a regression with this change, or is it unrelated?

FlorianUekermann · 2017-12-02T10:15:32Z

I was wondering why there are changes to compiler/natives/fs_vfsdata.go in both commits. I don't really know that part of the codebase, so apologies if it should be obvious.

hajimehoshi · 2017-12-02T11:41:04Z

Is that a regression with this change, or is it unrelated?

Not sure since SetMapIndex crashes without my PR in the first place...

hajimehoshi · 2017-12-02T11:42:27Z

I was wondering why there are changes to compiler/natives/fs_vfsdata.go in both commits.

Fixing some Go files requires regenerating fs_vfsdata.go, and reflect.go is such Go file AFAIK.

hajimehoshi · 2017-12-28T19:53:14Z

What's going on? Probably you are working on WebAssembly version, which is great, but I'm worried that this GopherJS is not updated recently.

neelance · 2017-12-28T22:11:25Z

Yes, I am working on the WebAssembly backend for the Go compiler (and other work). Unfortunately this means that I have almost no time left for GopherJS. I feel like GopherJS is in a good enough state to have it further maintained by the community.

TestReflectSetMapIndex seems to pass on CircleCI.

hajimehoshi · 2017-12-30T18:45:19Z

I feel like GopherJS is in a good enough state to have it further maintained by the community.

Hmm, but recently GopherJS is not updated, although there are reported bugs and sent PRs?

neelance · 2017-12-30T20:35:14Z

@hajimehoshi Would you be open to helping with maintaining this project? I could give you commit permissions. @shurcooL already has them, but maybe he is also quite busy atm. ;-)

hajimehoshi · 2017-12-31T13:00:25Z

I'll be glad to help this project :-)

I'm afraid I can't do such quick reviews as you did before for a while, but I'd like to try to do my best.

neelance · 2017-12-31T13:04:25Z

@shurcooL Wdyt? I think you two should talk a bit to discuss how you want to go about it.

dmitshur · 2018-01-11T04:02:50Z

@hajimehoshi Help with maintaining this project would be greatly appreciated, thanks for the offer. I'll want to discuss it in more detail with you on Slack, hopefully soon.

But first let's focus on this PR and the issue(s) it tries to solve. I want to post an (incomplete) update about that.

The PR is short and simple, but the issues it tries to solve are difficult. To fix them, it requires good understanding of the type system. That's something I never had a chance to really spend time on fully understanding (and it's not well documented), which makes working on this harder and more time intensive for me.

I ended up spending the entire day (!) today on this issue and better understanding it, the relevant concepts of GopherJS type system implementation, and what would be a good overall fix. (In the process, I also found and resolved another issue in reflect package on my WIP 1.10 branch.) That is one of the the main reasons for my delay in getting back; I haven't had the chance to dedicate such a large uninterrupted block of time to this until today, sorry about that.

The day was enough to make significant progress on this, but not enough to finish. I'll resume tomorrow. We can talk more about how to move forward.

The summary is that... This PR makes a small change that's acceptable, but it's not enough to fix #662 and #717. To really fix those issues (and I have semi-working prototypes of that), many additional changes are needed, and this PR may or may no longer be relevant. That's why I think it might actually make more sense to fix the entire issue at once and review that change; rather than making incremental incomplete steps. But I don't have objections to merging this PR as is either if you strongly prefer (just need to update PR description so it won't close those two issues).

hajimehoshi · 2018-01-12T03:21:59Z

I'm fine with waiting for fixing the root cause instead of merging my PR. I'd like to know your insights of GopherJS implemention :-)

Call only when possible at reflect.mapassign

f15070e

hajimehoshi changed the title ~~Call only when possible at reflect.mapassign~~ Call $get only when possible at reflect.mapassign Nov 7, 2017

dmitshur reviewed Nov 10, 2017

View reviewed changes

Use reflect package directly in the test

1ac50aa

hajimehoshi changed the title ~~Call $get only when possible at reflect.mapassign~~ reflect: avoid dereferencing non-pointers in mapassign Dec 1, 2017

flimzy assigned nevkontakte Dec 28, 2021

reflect: avoid dereferencing non-pointers in mapassign #718

Are you sure you want to change the base?

reflect: avoid dereferencing non-pointers in mapassign #718

Uh oh!

Conversation

hajimehoshi commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Nov 10, 2017

Uh oh!

dmitshur commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajimehoshi Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

hajimehoshi commented Nov 16, 2017

Uh oh!

hajimehoshi commented Nov 21, 2017

Uh oh!

dmitshur commented Nov 21, 2017

Uh oh!

hajimehoshi commented Nov 22, 2017

Uh oh!

FlorianUekermann commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Dec 2, 2017

Uh oh!

hajimehoshi commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Dec 2, 2017

Uh oh!

FlorianUekermann commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Dec 2, 2017

Uh oh!

hajimehoshi commented Dec 2, 2017

Uh oh!

hajimehoshi commented Dec 28, 2017

Uh oh!

neelance commented Dec 28, 2017

Uh oh!

hajimehoshi commented Dec 30, 2017

Uh oh!

neelance commented Dec 30, 2017

Uh oh!

hajimehoshi commented Dec 31, 2017

Uh oh!

neelance commented Dec 31, 2017

Uh oh!

dmitshur commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Jan 12, 2018

Uh oh!

Uh oh!

hajimehoshi commented Nov 7, 2017 •

edited

Loading

dmitshur commented Nov 10, 2017 •

edited

Loading

hajimehoshi commented Nov 10, 2017 •

edited

Loading

dmitshur Nov 10, 2017 •

edited

Loading

FlorianUekermann commented Dec 1, 2017 •

edited

Loading

hajimehoshi commented Dec 2, 2017 •

edited

Loading

FlorianUekermann commented Dec 2, 2017 •

edited

Loading

dmitshur commented Jan 11, 2018 •

edited

Loading