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

reflect: avoid dereferencing non-pointers in mapassign #718

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@hajimehoshi
Member

hajimehoshi commented Nov 7, 2017

Fixes #662 and #717

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

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

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

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi
Member

hajimehoshi commented Nov 10, 2017

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 10, 2017

Member

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.

Member

dmitshur commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Nov 10, 2017

Member

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.

Member

hajimehoshi commented Nov 10, 2017

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.

Show outdated Hide outdated tests/misc_test.go Outdated
@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi
Member

hajimehoshi commented Nov 16, 2017

ping

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Nov 21, 2017

Member

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

Member

hajimehoshi commented Nov 21, 2017

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

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 21, 2017

Member

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

Member

dmitshur commented Nov 21, 2017

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

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Nov 22, 2017

Member

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

Member

hajimehoshi commented Nov 22, 2017

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

@FlorianUekermann

This comment has been minimized.

Show comment
Hide comment
@FlorianUekermann

FlorianUekermann Dec 1, 2017

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.

FlorianUekermann commented Dec 1, 2017

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 hajimehoshi changed the title from Call $get only when possible at reflect.mapassign to reflect: avoid dereferencing non-pointers in mapassign Dec 1, 2017

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 2, 2017

Member

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.

Member

hajimehoshi commented Dec 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 2, 2017

Member

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.

Member

hajimehoshi commented Dec 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 2, 2017

Member

m.MapIndex(k) crashes

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

Member

dmitshur commented Dec 2, 2017

m.MapIndex(k) crashes

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

@FlorianUekermann

This comment has been minimized.

Show comment
Hide comment
@FlorianUekermann

FlorianUekermann Dec 2, 2017

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.

FlorianUekermann commented Dec 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 2, 2017

Member

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

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

Member

hajimehoshi commented Dec 2, 2017

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

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

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 2, 2017

Member

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.

Member

hajimehoshi commented Dec 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 28, 2017

Member

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.

Member

hajimehoshi commented Dec 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Dec 28, 2017

Member

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.

Member

neelance commented Dec 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 30, 2017

Member

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?

Member

hajimehoshi commented Dec 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Dec 30, 2017

Member

@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. ;-)

Member

neelance commented Dec 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Dec 31, 2017

Member

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.

Member

hajimehoshi commented Dec 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Dec 31, 2017

Member

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

Member

neelance commented Dec 31, 2017

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

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 11, 2018

Member

@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).

Member

dmitshur commented Jan 11, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Jan 12, 2018

Member

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 :-)

Member

hajimehoshi commented Jan 12, 2018

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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment