pthom / imgui_bundle

Dear ImGui Bundle: an extensive set of Ready-to-use widgets and libraries, based on ImGui. Start your first app in 5 lines of code, or less. Whether you prefer Python or C++, this pack has your back!
https://pthom.github.io/imgui_bundle/
MIT License
592 stars 63 forks source link

Memory access problem with 1.2.1 with Python in Linux #170

Closed Johndeep closed 5 months ago

Johndeep commented 5 months ago

Hi everyone,

after roughly checking this issue for duplicates or similarities, I came to the conclusion to try to post this.

Just to be clear, I work with Python in Ubuntu 22.04 LTS in Wayland. But I also tested this issue in Xorg. Unfortunately same result.

After updating imgui-bundle from 1.1.0 to 1.2.1 via pip, I noticed that the line imgui.create_context() leads to a crash of my app. Now the curious part.

The app starts briefly in all cases. It shows no content. However, it either stops without any terminal output or outputs a memory access error line, which is not helpful.

I'm working on a system with Intel core i7-1365U, which comes with Iris Xe and thus in Ubuntu with Mesa Intel Graphics (RPL-P) driver. I think this is quite strange, since imgui-bundle 1.1.0 works fine.

Currently, I'm working with version 1.1.0 until this is resolved.

This is quite a bummer actually, since I really like to use the new functionality of ChildFlags to make the child area re-sizable.

pthom commented 5 months ago

Hello,

Thanks for reaching out. I cannot reproduce your issue here, so I will need your help to investigate.

Python side

Could you confirm that this fails:

from imgui_bundle import imgui
imgui.create_context()

If so, this is probably something inside imgui (or in the bindings around it).

Can you please:

C++ side

Could you clone https://github.com/pthom/imgui_bundle_template and build it (it is a simple hello world with imgui bundle in C++) and tell me if it works?

# clone
https://github.com/pthom/imgui_bundle_template.git
cd imgui_bundle_template
# build
mkdir -p build && cd build
cmake ..
make -j 4
# Run
./imgui_bundle_example_integration

Follow up

If the problem persists, since I cannot reproduce it on my side, please contact me by email at pthomet at gmail dot com, I'll try to help by email.

Johndeep commented 5 months ago

Thank you for the very fast reply! Thus, I'll do my best to answer the effort.

Python side

  1. The following code (pip version) fails with memory access error..
    from imgui_bundle import imgui
    imgui.create_context()
  2. Reboot doesn't work either. Since it was the first thing I did after updating. Sorry for not mentioned it.
  3. The installation from source does work. (Btw. did version 1.2.1 added impl.process_event? I wondered that clicking didn't work until I've added that. Is that something ocornut added/separated from process_inputs? Sorry for not being up-to-date)

C++ side

I did all those steps and it also seems to work. To make sure it is the right version, I've added the child windows snippet with the variable resizing. It works as intended.

Preliminary Conclusion

It seems the imgui-bundle version 1.2.1 on pip is currently faulty, since the compiled version directly from the source does work. I don't need to mention that the C++ version also has no problems.

pthom commented 5 months ago

I cannot replicate your issue on Ubuntu22.04 (but I'm using arm64 on a Mac). I'm afraid the wheel build might be faulty, although I don't know why.

If you have time could you try to install from source at tag v1.2.1:

cd imgui_bundle
git checkout v1.2.1
git submodule update
pip install -v .

If it also works, I'll have to suspect the wheels built by github's CI...

Btw. did version 1.2.1 added impl.process_event? I wondered that clicking didn't work until I've added that. Is that something ocornut added/separated from process_inputs? Sorry for not being up-to-date

I did not understand what you are referring to. Can you clarify?

Child windows snippet with the variable resizing.

Is this a new flag in ImGui?

Johndeep commented 5 months ago

Now I had the chance to test this issue on another notebook, which is a Intel core i5-7200U, which entails an older Intel HD Graphics.

I tested it again with the pip version 1.2.1, but I also had the same issues. Namely the same memory access error as on the other machine.. Of course I also tried to compile everything from source (older CPU, more tea time I guess), which was again the same. I successfully got imgui-bundle to work with the main branch.

I tried what you suggested and planned to build against v1.2.1 by checking out that branch (or tag, whatever). But I could not. Rather, an error just popped up after trying to pip install -v .

  error: subprocess-exited-with-error

  × Building wheel for imgui-bundle (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> See above for output.

  note: This error originates from a subprocess, and is likely not a problem with pip.
  full command: /usr/bin/python3 /usr/lib/python3/dist-packages/pip/_vendor/pep517/in_process/_in_process.py build_wheel /tmp/tmpy0okxmmq
  cwd: [..]/imgui_bundle
  Building wheel for imgui-bundle (pyproject.toml) ... error
  ERROR: Failed building wheel for imgui-bundle
Failed to build imgui-bundle
ERROR: Could not build wheels for imgui-bundle, which is required to install pyproject.toml-based projects

You may be right about the wheels.. and just for the record, I successfully build the entire thing with the main branch.


About the impl.process_event. I use the sdl2-backend combination. In sdl2 impl.process_inputs() needs to be called for any inputs (mouse and key) to be recognized by imgui at all. But after updating to v1.2.1, I need to call impl.process_event(*event) instead. But since you're not aware of this, I assume that this is rather a change done in the main imgui branch I guess.

About Child Flags. Yes, this seems to be rather new, since ocornut implemented it just last year in summer it seems. With this, the parameter for imgui.beginchild changes slightly. Previously, one parameter was to determine, if it has a drawn border or not. Now it is integrated into the flags. It can be set by `imgui.ChildFlags.border. The new thing is, that you can also setimgui.ChildFlags_.resize_x` for example. This allows you to resize the child directly by dragging the border on the right.

child_border_drag.webm

pthom commented 5 months ago

Now I had the chance to test this issue on another notebook, which is a Intel core i5-7200U, which entails an older Intel HD Graphics. I tested it again with the pip version 1.2.1, but I also had the same issues. Namely the same memory access error as on the other machine..

Hum, this is getting worrying.

I tried what you suggested and planned to build against v1.2.1 by checking out that branch (or tag, whatever). But I could not. Rather, an error just popped up after trying to pip install -v .

If a source build fails, this is also an issue... The log extract you inserted is not long enough and does not contain the root cause of the error. Can you please attach the full log, or a longer extract where the root cause is displayed.

About the impl.process_event. I use the sdl2-backend combination. In sdl2 impl.process_inputs() needs to be called for any inputs (mouse and key) to be recognized by imgui at all. But after updating to v1.2.1, I need to call impl.process_event(*event) instead. But since you're not aware of this, I assume that this is rather a change done in the main imgui branch I guess.

Do you mean this, when using a pure python backend?


As far as wheels built by github are concerned, I'd like to make a test: each time a modification is made on the main branch wheels are built by github. They contain an artifact which you can download, for example here, and this artifact contains wheels for all platforms.

Could you tell me if the wheels in this artifact work for you?

Johndeep commented 5 months ago

Hi there again.

I tried to reproduce the installation of the git repo with v1.2.1 again. On the newer machine today, strangely enough, it could compile without problems. So for verification, I plan to do the same with the other machine again later today, since I don't have access to it right now. But since both machine are similar (Intel 64bit etc.), I suspect being sloppy myself yesterday for not cleaning up the build. But that would not explain why, without cleanup, a git checkout to the latest branch in the very same folder worked fine again though.

Sorry for currently not being able to provide the full log from yesterday, since I just copied it from terminal. (Forgot the proper manner of full logs)

I assume there weren't any changes since yesterday with the v1.2.1 tag?

Anyway, after the successful installation (demo says version 1.19.0), imgui-bundle works just fine. There I've wondered if I did something wrong yesterday and tried to install imgui-bundle from pip directly again. But sadly, it still crashes like yesterday.. I could reproduce the error from yesterday..


Yes exactly, I mean that pure python backend snippet! Line 40 and 41. It is strange though. I guess line 41 impl.process_inputs() could be obsolete now? I did tested it with the show_demo_window line, but every input works just fine without line 41.


About the wheels, I downloaded the example artifact from the link you've provided (#586). And just as I'm writing this line, the compilation has finished. All the example codes works just fine.

pthom commented 5 months ago

...the compilation has finished. All the example codes works just fine.

Actually, I would like you to try without compiling, by using the compiled wheel that corresponds to your python version. i.e. you could use either:

artifact/imgui_bundle-1.2.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
artifact/imgui_bundle-1.2.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
artifact/imgui_bundle-1.2.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

On the newer machine today, strangely enough, it could compile without problems

Sometimes, a previous compilation left temp files inside _skbuild, and removing this folder before recompiling sometimes avoids errors.


Yes exactly, I mean that pure python backend snippet! Line 40 and 41. It is strange though. I guess line 41 impl.process_inputs() could be obsolete now? I did tested it with the show_demo_window line, but every input works just fine without line 41.

Well, the python backends are almost direct adaptions from pyimgui bindings.

The sdl backend for pyimgui also provide process_inputs, and inside imgui_bundle process_inputs was adapted from it.

I removed the mouse handling from it, since it was done in process_events. However it still has some impact on imgui (set io.display_size, io.display_framebuffer_scale and io.delta_time)

Johndeep commented 5 months ago

Okay, without compilation. No problem. I still have Ubuntu 22.04 with Python 3.10, I went by cp310.

Now I understand what you want from me. So I went on the journey, went by divide and conquer and downloaded around 11 artifacts in total. I could pin down the problem to be in between artifact 528 and 531. As you can see, 529 and 530 were not build due to errors.

So artifact 528 is the last package, where imgui-bundle actually works (for Ubuntu). From 531 until the latest (which is 586 as of now), I get the memory access error. So I tried to compare the source folders to see what differences happened, and the following might be the cause:

grafik

That is what I noticed. You can try to analyze it yourself to what to do about that. I have no knowledge about the automatic builds in github (I should invest some time for that though), but I would be glad, if you figure it out and could explain what the problem was.

pthom commented 5 months ago

Woo, many thanks for this analysis.

So, after analysing the artifacts numbers, the possible commits that cause the error are:

14faa56 update hello_imgui [Pascal Thomet] ([hello_imgui] updated from e8ec91 to 0b5027)

b722ca9 Update ImGui bindings: publish ImGuiInputTextCallback and ImGuiSizeCallback as std::function [Pascal Thomet]

191532f imgui: publish IniFileName and LogFilename as string [Pascal Thomet]

I would suspect 14faa56, but I cannot find the related history in hello_imgui. Something strange...

Anyhow, I will have to publish an update on pypi, since the latest is broken for ubuntu users. Many thanks to you!

As I cannot reliably test this on my side (as it seems), can you please check wait until the latest wheels build is finished (now compiling, and should be ready in 1 hour), and tell me if the binary wheels work for you.

If so, I'll try to prepare a release based on this.

pthom commented 5 months ago

PS: if you have something to show, I'm always pleased to see what users are doing with imgui_bundle in the gallery

Johndeep commented 5 months ago

Okay, just downloaded the newest artifact 587 and tried it out. But unfortunately, the binary wheel didn't work. Same error (memory access error, or if executed from vscodium, no output at all, silent exit so to say). And as always for control, either self compiled or version 1.1.0 do work.

PS: Currently not yet, but if my experimenting phase is done and I've something developed, I could show. Since this is academic work, it might be possible to let you have a glance on it in the future. Anyway, I should be very thankful that you bundled up imgui and some really useful "batteries" like ImPlot or Node-Editor into this for python! Else I would consider to develop in C++ or something.. or not using any GUI at all.

Johndeep commented 5 months ago

I know this little post is not really related to this issue, but the reason for me using imgui-bundle has something to do due to pyimgui not working on Wayland at all. Due to OpenGL.error.Error: Attempt to retrieve context when no valid context error pyimgui #318. None of the suggested workarounds there work for me.

Also, they are a tad behind the updates from ocornut directly (would not be a big deal since I didn't use the new features very much). Their version 2.0.0 is currently based on 1.82. And there are currently no sign of any updates since 9 month ago (as of writing now).

pthom commented 5 months ago

Hello,

Are you available for a session where we could work together on this (we would setup a conf call with screen sharing, so that I can try to diagnose more in details).

I could be available at 4PM paris time today, i.e 3PM UTC.

pthom commented 5 months ago

If not, I'll need to setup an Intel Linux machine somewhere

Johndeep commented 5 months ago

Unfortunately, I'm currently not available for calls due to work.. Later I may try to replicate by forking and looking for some clues in between the artifacts. Sorry for that.

pthom commented 5 months ago

I understand

pthom commented 5 months ago

The issue seems to come from cibuildwheel.

Here are my findings:

Below is a draft for a message I intend to post to cibuildwheel


I have an issue with a wheel built on github runners when I try to install it on ubuntu 22.04.

A simple call to new fails when using the wheel, however it does work when installing from source via pip install -v .

The wheel is named imgui_bundle-1.2.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl, i.e. it is a manylinux wheel for python 3.10 x86_64.

About the wheel:

The wheel was built like this (this is a copy-paste of the GH action provided by pypa/cibuildwheel@v2.16.2):

pipx run --python '/opt/hostedtoolcache/Python/3.11.7/x64/bin/python' --spec '/home/runner/work/_actions/pypa/cibuildwheel/v2.16.2' cibuildwheel "." --output-dir "wheelhouse" --config-file "" --only "" 2>&1

When I install it on ubuntu 22, with

pip install wheelhouse/imgui_bundle-1.2.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

I get a segmentation fault, if I call a simple function (that mainly allocates a new object via new):

python -c "from imgui_bundle import imgui; imgui.create_context()"

The output is:

ImGui::CreateContext 1
ImGui::CreateContext 2
sizeof ImGuiContext = 27792
Segmentation fault (core dumped)

I have instrumented the for ImGui::CreateContext(), and we see that a simple call to new fails !!!!

ImGuiContext* ImGui::CreateContext(ImFontAtlas* shared_font_atlas)
{
    printf("ImGui::CreateContext 1\n");
    ImGuiContext* prev_ctx = GetCurrentContext();
    printf("ImGui::CreateContext 2\n");
    printf("sizeof ImGuiContext = %lu\n", sizeof(ImGuiContext));
    ImGuiContext* ctx = new ImGuiContext(shared_font_atlas);  //IM_NEW(ImGuiContext)(shared_font_atlas);
    printf("ImGui::CreateContext 3\n");
    SetCurrentContext(ctx);
   ...

About the local install via pip:

Note: when building the package locally, with

pip install -v .

It works correctly!

python -c "from imgui_bundle import imgui; imgui.create_context()"
    ImGui::CreateContext 1
    ImGui::CreateContext 2
    sizeof ImGuiContext = 27840
    ImGui::CreateContext 3

Notable differences:

sizeof ImGuiContext = 27792    with the wheel binary
sizeof ImGuiContext = 27840    with the local pip install

This can be reproduced with the repository https://github.com/pthom/imgui_bundle, on the branch dbg_ub and it can happen directly on the ubuntu-latest runner from GH.

Am I doing something wrong?

Johndeep commented 5 months ago

First time hearing about this cbuildwheel. This seems to.. ease cross compilation? As I were tinkering around in the wheel file, I notice this cbuildwheel line. So that is where it comes from..? So the builds are relying on other projects?

Since I'm no expert in this matter, I trust you that this will lead to the culprit. But the reasoning makes sense. Segfaults or memory access problems usually comes from somewhere in wrong bindings of video driver related library, I guess?

pthom commented 5 months ago

Update: I confirm this is a cibuildwheel issue: see https://github.com/pypa/cibuildwheel/issues/1724

As a workaround, I can disable the usage of std::string for IniFilename and LogFilename. It should hopefully be sufficient.

Wheels are being built with this change. Can you test them?

Johndeep commented 5 months ago

I can confirm, the change of this artifact you've done here, is working! For record, in this build, imgui is at 1.90.1, which should be the most recent what ocornut also just released.

pthom commented 5 months ago

This was my most difficult bug hunt ever. And I only found a workaround. Let's hope it gets fixed in cibuildwheel, or if I made a mistake that I get to know where it was.

Johndeep commented 5 months ago

I can't even understand how you got to the conclusion of a cibuildwheel bug and also as to why not using std::string can make everything work again as a workaround. Seems quite unrelated to me.. Anyway, great job!

Are there no simple tests running after workflow builds are done to see, for example, if a code like from imgui_bundle import imgui; imgui.create_context() works on every platform?

I mean yes, in the .github folder, there are various yaml (quite a lot actually) for building it seems. So the ci_automation_tests didn't seem to cover that..?

Soo, since this is solved by a workaround, can this issue be closed or should it stay open for reference?

pthom commented 5 months ago

using std::string can make everything work again as a workaround

When sizeof() gives different sizes, you enter a world of pain; because you are probably facing ABI incompatibilities. I suspected the std lib (and thus std::string).

how you got to the conclusion of a cibuildwheel bug

It was long, I had to check on multiple platforms, and also check that a standard binary wheel build does not have the issue

So the ci_automation_tests didn't seem to cover that..?

No, ci_automation_tests runs a complete automated application test after pip install -v .. I never suspected that cibuildwheel might give a result that is different from pip install...

running after workflow builds are done to see, for example, if a code like from imgui_bundle import imgui; imgui.create_context() works on every platform

Yes, I added this in this commit. I cannot do more tests with cibuildwheel, because adding a X server to run the tests with cibuildwheel would get ultra complicated.

Soo, since this is solved by a workaround, can this issue be closed or should it stay open for reference?

I'll leave it open for reference for a while

pthom commented 5 months ago

I released version 1.3.0 to correct the wheels for linux users:

Based on my tests, it works. Can you confirm?

Johndeep commented 5 months ago

I just made a pip install imgui-bundle -U to update to 1.3.0. I can confirm that I can successfully run the sdl2, glfw and helloimgui examples.

pthom commented 5 months ago

I found the reason for the bug, and actually it's deep in the roots of Linux systems.

See my last answer here before I closed the issue, and reopened another which is more to the point:

A wheel built with pip install

> file _mylib.cpython-310-x86_64-linux-gnu.so
 ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked

A wheel built with cibuildwheel

> file _mylib.cpython-310-x86_64-linux-gnu.so
 ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked

And std::string from the GNU/Linux ABI does not support zero-initialization, which ImGui uses a lot, like below:

    IO::IO()
    {
        // ImGui uses many C-style initializations techniques, such as this memset
        // However, the std::string provided by GNU/Linux does *not* support to be zero-initialized!!!
        memset(this, 0, sizeof(*this));

        ...

        IniFilename = "imgui.ini";  // This fails spectacularly with GNU/Linux ABI if IniFilename is a std::string
    }
Johndeep commented 5 months ago

What dedication!

But also the root of the problem caught me off-guard. It goes deeper and is less straight forward that I though. To be honest, I never really used memset. Now with a bit of research, since it sounds important to know, this is one way to initialize a potentially large memory section like a struct. Because it is fast and can "initialize" the object quickly with a certain value.

But it seems that this should be avoided in C++, since it circumvents the type safety that C++ provides (by the way, imgui is around 88% C++). Since you mentioned that this sort of std::string is used alot in ImGui (not wonder, it is surely more convenient and safer than char* or similar).


Anyway, fascinating that two different compiler are used here. As I've found out, among others, clang, g++ and c++ on my Ubuntu are compiling with a SYSV type.

To test this again, I installed the imgui_bundle v1.2.1 via pip install -v .. The binary "_imgui_bundle.cpython-310-x86_64-linux-gnu" is compiled with the GNU/Linux type, but it works well. And just to be really sure, I did pip install imgui_bundle==1.2.1 to see the type:

# So I checked the _imgui_bundle.cpython-310-x86_64-linux-gnu.so file

# This is installed via pip install -v . (local compilation), works well
ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, 
[...], stripped, size 42.595.432 Bytes

# This lib installed from pip leads to the error.
ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, 
[...], stripped, size 43.718.729 Bytes

I may be wrong, but I suspect a different version of the same compiler. Or different flags or whatever. But yes, let me stop here, this issue is solved, these are only afterthoughts.