cogentcore / core

A free and open source framework for building powerful, fast, and elegant 2D and 3D apps that run on macOS, Windows, Linux, iOS, Android, and the Web with a single pure Go codebase, allowing you to Code Once, Run Everywhere.
http://cogentcore.org/core
BSD 3-Clause "New" or "Revised" License
1.31k stars 71 forks source link

gtigen panic #926

Closed ddkwork closed 3 months ago

ddkwork commented 4 months ago

Describe the bug

nil point

How to reproduce

go test https://github.com/cogentcore/core/blob/main/gti/gtigen/gtigen_test.go

Example code

No response

Relevant output

GOROOT=C:\Program Files\Go #gosetup
GOPATH=C:\Users\Admin\go #gosetup
"C:\Program Files\Go\bin\go.exe" test -c -o C:\Users\Admin\AppData\Local\JetBrains\GoLand2023.3\tmp\GoLand\___gtigen_test_go.test.exe cogentcore.org/core/gti/gtigen #gosetup
"C:\Program Files\Go\bin\go.exe" tool test2json -t C:\Users\Admin\AppData\Local\JetBrains\GoLand2023.3\tmp\GoLand\___gtigen_test_go.test.exe -test.v -test.paniconexit0 -test.run ^\QTestGenerate\E|\QTestPerson\E$ #gosetup
=== RUN   TestGenerate
--- FAIL: TestGenerate (2.34s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x10 pc=0xb2d4ed]

goroutine 6 [running]:
testing.tRunner.func1.2({0xb7a140, 0xeee430})
    C:/Program Files/Go/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
    C:/Program Files/Go/src/testing/testing.go:1634 +0x377
panic({0xb7a140?, 0xeee430?})
    C:/Program Files/Go/src/runtime/panic.go:770 +0x132
cogentcore.org/core/gti/gtigen.(*Generator).InspectGenDecl(0xc00a7de1c0, 0xc00a3f0d00)
    C:/Users/Admin/Desktop/core/gti/gtigen/generator.go:207 +0x94d
cogentcore.org/core/gti/gtigen.(*Generator).Inspect(0xc0002e0360?, {0xc8c9e8?, 0xc00a3f0d00?})
    C:/Users/Admin/Desktop/core/gti/gtigen/generator.go:127 +0xde
cogentcore.org/core/gengo.Inspect.func1({0xc8c9e8, 0xc00a3f0d00})
    C:/Users/Admin/Desktop/core/gengo/gengo.go:74 +0x47
go/ast.inspector.Visit(0xc00a82d7a0, {0xc8c9e8?, 0xc00a3f0d00?})
    C:/Program Files/Go/src/go/ast/walk.go:386 +0x2b
go/ast.Walk({0xc8bba0?, 0xc00a82d7a0?}, {0xc8c9e8, 0xc00a3f0d00})
    C:/Program Files/Go/src/go/ast/walk.go:51 +0x4c
go/ast.walkDeclList(...)
    C:/Program Files/Go/src/go/ast/walk.go:38
go/ast.Walk({0xc8bba0?, 0xc00a82d7a0?}, {0xc8ca38, 0xc009e694a0})
    C:/Program Files/Go/src/go/ast/walk.go:366 +0x36c5
go/ast.Inspect(...)
    C:/Program Files/Go/src/go/ast/walk.go:397
cogentcore.org/core/gengo.Inspect(0xc00a7de1c0?, 0xc00a7f5e40)

    C:/Users/Admin/Desktop/core/gengo/gengo.go:70 +0x119
cogentcore.org/core/gti/gtigen.(*Generator).Find(0xc00a7de1c0)

    C:/Users/Admin/Desktop/core/gti/gtigen/generator.go:83 +0x99
cogentcore.org/core/gti/gtigen.GeneratePkgs(0xc000067ae0, {0xc00a7c91d0, 0x1, 0x1})
    C:/Users/Admin/Desktop/core/gti/gtigen/gtigen.go:63 +0x113
cogentcore.org/core/gti/gtigen.Generate(0xc000067ae0)
    C:/Users/Admin/Desktop/core/gti/gtigen/gtigen.go:48 +0x3c
cogentcore.org/core/gti/gtigen.TestGenerate(0xc0001271e0)
    C:/Users/Admin/Desktop/core/gti/gtigen/gtigen_test.go:44 +0x3d0
testing.tRunner(0xc0001271e0, 0xc176b8)
    C:/Program Files/Go/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 1
    C:/Program Files/Go/src/testing/testing.go:1742 +0x390

Platform

Windows

ddkwork commented 4 months ago

we need change InspectGenDecl func as:

            for i, field := range st.Fields.List {
                if field == nil {//---------------------------------------> add this 
                    continue
                }
                // if we have no names, we are embed, so add to embeds and remove from fields
                if len(field.Names) == 0 {
                    emblist.List = append(emblist.List, field)
                    st.Fields.List = slices.Delete(st.Fields.List, i+delOff, i+1+delOff) // we need to add delOff to correctly handle situations where we delete multiple times and our indices become inaccurate
                    delOff--                                                             // we have deleted so we need to update offset
                }
            }
kkoreilly commented 4 months ago

I can not reproduce this issue; are you doing anything other than just running go test in gtigen?

kkoreilly commented 3 months ago

The reason that the test is failing is that you are running it outside of the normal location of the code, which means that it can not load the correct files for the test, and thus it panics. I do not know why GoLand decides to put everything in bizarre temporary locations, as that is the cause of both this issue and the Vulkan panic. This will work if you run the test in the normal way by just running go test in gtigen, so this is not an issue. We will not support running tests in the wrong location, and the test works perfectly with both the standard command line workflow and VS Code. You can probably configure GoLand to place executables and tests in the current directory. Adding a pointless nil check does not fix this problem; you are conflating not panicking and working: just because something doesn't panic doesn't mean that it works. Most of the nil panic issues are caused by other problems, and adding a simple nil check almost never fixes the actual problem.

ddkwork commented 3 months ago

Ok

ddkwork commented 3 months ago
{19DC250D-FDB0-4859-B319-DE34EE770600}
ddkwork commented 3 months ago

I have a different take on this issue.

ddkwork commented 3 months ago
            for i := 0; i < len(st.Fields.List); i++ {
                field := st.Fields.List[i]
                // if we have no names, we are embed, so add to embeds and remove from fields
                if len(field.Names) == 0 {
                    emblist.List = append(emblist.List, field)
                    st.Fields.List = slices.Delete(st.Fields.List, i+delOff, i+1+delOff) // we need to add delOff to correctly handle situations where we delete multiple times and our indices become inaccurate
                    i++
                    delOff-- // we have deleted so we need to update offset
                }
            }
ddkwork commented 3 months ago

As for vulkan, as long as you've successfully mocked the various events inside the unit tests, I'll patiently debug it and see if I can find the cause of the actual panic. But between the invisibility of cgo code to debugging, I'm not sure, unless there's an alternative way to intercept the cgo execution flow

kkoreilly commented 3 months ago

Please tell me whether the gtigen test still panics when you run it in Git Bash with go test.

ddkwork commented 3 months ago

I have tried, external run cmd, go test is also panic, another code inside the logic and execution process I have verified!

2762713521 @.***

 

------------------ 原始邮件 ------------------ 发件人: "cogentcore/core" @.>; 发送时间: 2024年3月6日(星期三) 晚上11:45 @.>; @.**@.>; 主题: Re: [cogentcore/core] gtigen panic (Issue #926)

Please tell me whether the gtigen test still panics when you run it in Git Bash with go test.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

kkoreilly commented 3 months ago

Please send me a screenshot of you running go test in gtigen.

ddkwork commented 3 months ago
{5D2349C6-FE1C-4b8f-BC74-510FF7FEE0C6}
kkoreilly commented 3 months ago

It is really weird that it panics on your computer but not mine. I will work on debugging this now.

ddkwork commented 3 months ago

You write the code you should be very clear about the logic of its implementation, slice 5 members, deleted a, the remaining 4, range to the fourth time, 4 is index, starting from 0, in fact, he is 5, but has been deleted, so the empty pointer panic

ddkwork commented 3 months ago
          for i := 0; i < len(st.Fields.List); i++ {
              field := st.Fields.List[i]
              // if we have no names, we are embed, so add to embeds and remove from fields
              if len(field.Names) == 0 {
                  emblist.List = append(emblist.List, field)
                  st.Fields.List = slices.Delete(st.Fields.List, i+delOff, i+1+delOff) // we need to add delOff to correctly handle situations where we delete multiple times and our indices become inaccurate
                  i++
                  delOff-- // we have deleted so we need to update offset
              }
          }

This fix perfectly matches the execution logic of your code

kkoreilly commented 3 months ago

Sorry, I misunderstood the situation, and I will apply that fix now.

ddkwork commented 3 months ago

I would like to ask you a question, I'm trying to understand the execution of core by debugging, but I have not been able to find the place where the reflected structure is. For example:

func TestStructView(t *testing.T) {
    b := gi.NewBody()
    NewStructView(b).SetStruct(myStructValue)
    b.AssertRender(t, "structview/basic")
}

As I understand it, SetStruct is supposed to perform reflection, i.e.: reflect the struct to the ki n-forked tree structure, then various traversals to add events, styles, etc. to send to vulkan, and finally vulkan hands it off to glfw to interact with the graphics card hardware. But I didn't find any code related to the reflection structure.

kkoreilly commented 3 months ago

You are asking how the entire architecture of Cogent Core is structured, which is a hard question to easily answer. I will write architecture documentation that explains all of that soon, but until then, here is a simple answer: SetStruct results in StructView.ConfigWidget happening, in which the reflect.Value of the struct is used to construct a widget structure with labels and values for the fields of the struct. The struct view and the widgets add events and styles functions to the widgets, which are then called in later event loop and style application passes. All of the content is later rendered to an image on the Scene, which is then pushed to the screen with Vulkan.

kkoreilly commented 3 months ago

Can you try my new fix for this issue?

ddkwork commented 3 months ago

Sorry I just fell asleep for a while and it's 2:30 a.m. Thank you for the explanation of the reflection part. Also, I'm a little curious if you won't panic on your computer before this fix? Didn't you test? What's even stranger is that I didn't panic on my computer after running the core command for so long, perhaps for only one reason: except for this unit test scenario, all the core commands executed did not trigger names==0 to delete the slice member. It's just that when I execute the unit test, it crashes. In addition to this, the core command also has a warning under the giv package and exits, indicating that the field name is the same, which I guess is roughly the stake: the two fields of the struct have the same name. It should mean something similar.

ddkwork commented 3 months ago

It should be fine, wait a minute I'll test it. By the way, it feels like you're too busy to write code all day long

---Original--- From: @.> Date: Thu, Mar 7, 2024 02:13 AM To: @.>; Cc: @.**@.>; Subject: Re: [cogentcore/core] gtigen panic (Issue #926)

Can you try my new fix for this issue?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

ddkwork commented 3 months ago

So where did the drawing take place? Set up the struct, reflect, send the event, set the style, and then goosi receives the event and gets it to vulkan. I haven't seen where to execute the drawing operation after looking at the code related to this process for a long time, such as drawing the rectangle of the button, setting the foreground color after the mouse gets focus, etc., and I haven't found where the button widget is drawn after looking at it for a long time

---Original--- From: @.> Date: Thu, Mar 7, 2024 01:51 AM To: @.>; Cc: @.**@.>; Subject: Re: [cogentcore/core] gtigen panic (Issue #926)

You are asking how the entire architecture of Cogent Core is structured, which is a hard question to easily answer. I will write architecture documentation that explains all of that soon, but until then, here is a simple answer: SetStruct results in StructView.ConfigWidget happening, in which the reflect.Value of the struct is used to construct a widget structure with labels and values for the fields of the struct. The struct view and the widgets add events and styles functions to the widgets, which are then called in later event loop and style application passes. All of the content is later rendered to an image on the Scene, which is then pushed to the screen with Vulkan.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

ddkwork commented 3 months ago

Hello, I'm going to write an interface for ollama, but I don't know how to insert text into the text editing widget continuously without clearing the previous insertion, because the AI chat scene is the uninterrupted output of text, if the one-time output feels like the chat delay on a low-configuration device, so we need to support the stream feature, one input, streaming output. Is there a way to do this in the text editing widget?

ollama/ollama#2842

kkoreilly commented 3 months ago

The response from the AI should not be in a text editor; it should be in a Label. You can just call Label.SetTextUpdate with the newly streamed text whenever you want to.

ddkwork commented 3 months ago

Thanks, it's finally dawn and it's time to do code again

---Original--- From: @.> Date: Thu, Mar 7, 2024 08:08 AM To: @.>; Cc: @.**@.>; Subject: Re: [cogentcore/core] gtigen panic (Issue #926)

The response from the AI should not be in a text editor; it should be in a Label. You can just call Label.SetTextUpdate with the newly streamed text whenever you want to.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>