Opened 2 years ago

Closed 9 months ago

#13191 closed bug (fixed)

[Flurry] screensaver crashes in __strtod_internal

Reported by: diver Owned by: waddlesplash
Priority: normal Milestone: R1/beta1
Component: Add-Ons/Screen Savers Version: R1/Development
Keywords: Cc: kallisti5
Blocked By: Blocking: #14023
Has a Patch: yes Platform: All

Description

hrev50849 x86_64.

Selecting Flurry screensaver crashes ScreenSaver preflet.

Not reproducible in x86_gcc2.

Attachments (16)

ScreenSaver-9243-debug-10-01-2017-18-16-42.report (20.7 KB) - added by diver 2 years ago.
ScreenSaver-788-debug-16-01-2017-19-04-05.report (19.4 KB) - added by Premislaus 2 years ago.
Another crash report.
0001-Ensure-screen-savers-have-an-active-GL-context.patch (1.9 KB) - added by i80and 17 months ago.
GLInfo-647-debug-14-04-2018-14-20-17.report (25.3 KB) - added by cocobean 14 months ago.
GLInfo crash on exit
ScreenSaver-858-debug-14-04-2018-14-23-28.report (24.5 KB) - added by cocobean 14 months ago.
Flurry crash on hrev51877 x86_64
syslog (153.5 KB) - added by cocobean 14 months ago.
Syslog after crashes hrev51877 x86_64
haiku_flurry_x64.jpeg (225.7 KB) - added by cocobean 13 months ago.
Haiku hrev49856 x86_64 w/Mesa 10.5.2 working properly with GLife, GL Info, and Flurry.
ken-haiku51904-mesa-17.1.png (192.4 KB) - added by cocobean 13 months ago.
Mesa-swrast review on Haiku hrev51904 x86_64
paltex.c (7.1 KB) - added by cocobean 10 months ago.
Paltex test code
paltex (11.6 KB) - added by cocobean 10 months ago.
Paltex demo executeable (Haiku x86_gcc2)
morph3d.c (40.4 KB) - added by cocobean 10 months ago.
Morph3D test code
morph3d (26.7 KB) - added by cocobean 10 months ago.
Morph3D demo executeable (Haiku x86_gcc2)
flurry_haiku-hrev52138_x64-cocobean.jpeg (242.1 KB) - added by cocobean 10 months ago.
Haiku hrev52138 x86_64 w/Mesa 17.1.10 working properly with GLife, Gravity, and Flurry screensavers (swrast driver)
flurry_haiku-hrev52138_x64_test2_cocobean.jpeg (196.0 KB) - added by cocobean 10 months ago.
Haiku hrev52138 x86_64 w/Mesa 17.1.10 working properly with GLTeapot, Haiku3D demos and GLife (swrast driver) #2
Flurry (40.0 KB) - added by cocobean 10 months ago.
Flurry screensaver for Haiku hrev52153 x86_64 (fully tested/working)
haiku-flurry.patch (311 bytes) - added by cocobean 10 months ago.
Flurry screensaver patch based on i80and patch (tested on Haiku x86_64, mesa-swpipe 17.1.10)

Download all attachments as: .zip

Change History (59)

comment:1 Changed 2 years ago by diver

Owner: changed from nobody to kallisti5
Status: newassigned

Changed 2 years ago by Premislaus

Another crash report.

comment:2 Changed 17 months ago by i80and

This is because the FlurryView::AttachedToWindow() logic looks like the following:

void FlurryView::AttachedToWindow() {
  LockGL();
  BGLView::AttachedToWindow(); // Calls UnlockGL()
  // Do stuff assuming a context is active
}

When glGetString(GL_VERSION) is called, it returns a null string. The GLife screensaver has the same problem, but instead of crashing it just renders incorrectly.

For both Flurry and GLife, moving the LockGL call *after* the call to BGLView::AttachedToWindow fixes the problem.

I'll try to create a patch tomorrow.

comment:3 Changed 17 months ago by i80and

Has a Patch: set

comment:4 Changed 17 months ago by i80and

(Maybe BGLView::AttachedToWindow() shouldn't unconditionally unlock the GL context; I can persue that route instead if it's preferable)

comment:5 Changed 17 months ago by waddlesplash

What did BeOS do re. locking? We should try to mimic that.

comment:6 Changed 17 months ago by i80and

The BeBook seems to indicate that the existing behavior of the screensavers should be correct, meaning my patch should be rejected. The current BGLView::AttachedToWindow() locking logic was added to Haiku in fafc6e4b776ba7c9eff20696976d9fdb85113682 (2010) without explanation.

There are two options I can see:

  • Revert that part of the change, making Haiku match BeOS more closely based on the BeBook's code samples, but breaking applications written to depend on Haiku's current behavior.
  • Only call UnlockGL() iff the initial state of BGLView::fDisplayLocker::IsLocked() is false. That is, restore the original state right after setting up the glViewport.

I'll pursue the second option as a pull request to the mesa package in haikuports, if that's the right procedure?

comment:7 Changed 17 months ago by i80and

Has a Patch: unset

comment:8 Changed 17 months ago by waddlesplash

Cc: kallisti5 added

CC'ing kallisti5 as he's in charge of Mesa-related stuff.

Yes, that's probably correct. Note that there are 2 versions of Mesa though (Mesa 7 for GCC2 and current Mesa for GCC5+) and both will need to be patched I think.

comment:9 Changed 17 months ago by pulkomandy

Aren't LockGL() and UnlockGL() recursive locks? (sorry, I'm not familiar with the API)

If they are, then there should be no problem here - AttachedToWindow could safely call LockGL then UnlockGL which would restore the initial state of the lock.

A patch was submitted here: https://github.com/haikuports/haikuports/pull/2070/files Submitting directly to github.com/haiku/mesa (for gcc2) and upstream mesa (for gcc5) would be better than having the patch in haikuports.

comment:10 Changed 17 months ago by i80and

LockGL() and UnlockGL() currently don't behave recursively. I think they probably should?

I appreciate everybody taking the time to help me muddle my way through what a proper fix for this looks like :D

comment:11 Changed 17 months ago by waddlesplash

The Be Book does not specify whether or not they are recursive. Guess someone needs to fire up BeOS and find out...

comment:12 Changed 17 months ago by pulkomandy

Let's dig out that BeOS machine. Maybe someone has code I could easily test?

comment:13 Changed 15 months ago by jscipione

Blocking: 14023 added

comment:14 Changed 14 months ago by cocobean

I can assist work on a 'clean room' Mesa 17.1.10 re-implementation for Haiku - if we all come to the same conclusions.

Last edited 10 months ago by cocobean (previous) (diff)

comment:15 Changed 14 months ago by kallisti5

The core problem is people keep bumping the version of Mesa in our Haikuports repository without testing if it actually works. Mesa 12-15 were stable, but without the bandwith to work on it bugs creep in.

I've been trying to keep up, but real life and all the other Haiku work I have get in the way.

The SCons build system we use in Mesa is slowly being decommissioned. I've added Haiku support to their new Meson build system... but haven't had time to track down all the bugs.

Help wanted!

And *please* don't start talking of forking Mesa. The project moves fast... lets just not bump versions until we know that the version we bump to is stable + functional :-)

comment:16 Changed 14 months ago by pulkomandy

We need a release branch at haikuports and not let random people push/merge changes to it; at least for packages required for Haiku to run.

comment:17 Changed 14 months ago by cocobean

Last edited 14 months ago by cocobean (previous) (diff)

comment:18 Changed 14 months ago by cocobean

Issue (Haiku hrev51877 x86_64, Mesa 17.1.7-7):

Flurry - crashes before Screensaver preview.
GLinfo - Segmentation fault on exit with Mesa 17.1.7
GLife - Misrendering (cubes larger than normal)
Gravity - Misrendering (cubes larger than normal)
Last edited 10 months ago by cocobean (previous) (diff)

Changed 14 months ago by cocobean

GLInfo crash on exit

Changed 14 months ago by cocobean

Flurry crash on hrev51877 x86_64

Changed 14 months ago by cocobean

Attachment: syslog added

Syslog after crashes hrev51877 x86_64

comment:19 Changed 13 months ago by cocobean

See attachment: https://dev.haiku-os.org/attachment/ticket/13191/haiku_flurry_x64.jpeg

Confirming that GLife, GL Info, and Flurry all work, exit, and render properly on Haiku hrev49856 x86_64:

Software Rasterizer
Mesa Project
2.1 Mesa 10.5.2
GLU 1.3, GLU 1.5

IMPORTANT: Nightly ISO x86_64 image builds with Mesa 17.1.7 have the issue.

Last edited 10 months ago by cocobean (previous) (diff)

Changed 13 months ago by cocobean

Attachment: haiku_flurry_x64.jpeg added

Haiku hrev49856 x86_64 w/Mesa 10.5.2 working properly with GLife, GL Info, and Flurry.

Changed 13 months ago by cocobean

Mesa-swrast review on Haiku hrev51904 x86_64

comment:20 Changed 13 months ago by cocobean

Last edited 10 months ago by cocobean (previous) (diff)

comment:21 Changed 10 months ago by diver

Should we downgrade Mesa to 10.5.2 for beta1 to get rid of crashing bugs in the release?

comment:22 in reply to:  21 Changed 10 months ago by cocobean

Last edited 10 months ago by cocobean (previous) (diff)

comment:23 Changed 10 months ago by kallisti5

Should we downgrade Mesa to 10.5.2 for beta1 to get rid of crashing bugs in the release?

Yes.

10.5.2 was the last time I had a lot of free time to work on Mesa. Someone has been blindly bumping the version we use and not testing it.

Mesa moves fast, and we use a lot of non-public API calls to get the bare minimum done for rendering. The Mesa guys do their best with Haiku support, but without a full-time dev Haiku falls behind.

Last edited 10 months ago by kallisti5 (previous) (diff)

comment:24 Changed 10 months ago by waddlesplash

The reason we've been updating Mesa "blindly" is because of LLVM breaking its API with every major release. Mesa keeps up, but the old version will of course not work; so in order to go back we'd need to patch Mesa 10 to work with current LLVM. From the patches I've read, that is probably not an easy thing to do at all...

comment:25 Changed 10 months ago by cocobean

The Mesa 17.1.10 library is fine and tested for stability. Built on Haiku x86_gcc2 and x86_64.

Last edited 10 months ago by cocobean (previous) (diff)

comment:26 Changed 10 months ago by waddlesplash

Actually, it looks like this PR fixes the issue: https://github.com/haikuports/haikuports/pull/2070

comment:27 Changed 10 months ago by waddlesplash

Since the Be Book didn't say whether or not [Un]LockGL is recursive, and there's no way to determine if a BGLView is locked or not, I looked at the disassembly of R5's libGL. It seems that it's unlocking BLockers; probably these BLockers. However, it also seems that the context is not switched on UnlockGL, only on LockGL, or in other words, the last context stays current until a different LockGL is called.

But my assembly-fu is poor, so that could also be incorrect. At any rate, it seems a recursive lock is the way to go.

comment:28 Changed 10 months ago by waddlesplash

It seems that TinyGL's implementation of GLView was a recursive lock also: https://github.com/mojaves/TinyGL/blob/master/BeOS/GLView.cpp#L37

comment:29 Changed 10 months ago by jscipione

Ha you used my R5 headers! I wouldn't necessarily follow BeOS's implementation here in terms of locking.

comment:30 Changed 10 months ago by pulkomandy

We have to follow R5 closely, otherwise we run into compatibility problems, as people often used the locking in er... "creative" ways: unlocking from a different thread, recursive locking, ...

Rather than disassembling, we could also write a test app and run it on both R5 and Haiku. If you have something to test, I still have an R5 machine around for that purpose (as I already mentionned a few years ago in comment 12, but I still don't have any code to test :)).

Changed 10 months ago by cocobean

Attachment: paltex.c added

Paltex test code

Changed 10 months ago by cocobean

Attachment: paltex added

Paltex demo executeable (Haiku x86_gcc2)

comment:31 Changed 10 months ago by cocobean

Ref: #6400 - Upon program exit, some OpenGL programs not closing same thread as opened.

Changed 10 months ago by cocobean

Attachment: morph3d.c added

Morph3D test code

Changed 10 months ago by cocobean

Attachment: morph3d added

Morph3D demo executeable (Haiku x86_gcc2)

comment:32 Changed 10 months ago by waddlesplash

Yes that's to be expected and is not an issue.

comment:33 Changed 10 months ago by kallisti5

in morph3d.c, you might want to change #include "glut_wrapper.h" to #include <GL/freeglut.h> if testing.

Changed 10 months ago by cocobean

Haiku hrev52138 x86_64 w/Mesa 17.1.10 working properly with GLife, Gravity, and Flurry screensavers (swrast driver)

Changed 10 months ago by cocobean

Haiku hrev52138 x86_64 w/Mesa 17.1.10 working properly with GLTeapot, Haiku3D demos and GLife (swrast driver) #2

comment:34 Changed 10 months ago by cocobean

Last edited 10 months ago by cocobean (previous) (diff)

comment:35 Changed 10 months ago by waddlesplash

Please stop adding useless comments. The issue is already well-understood and only needs someone to sit down and code a fix.

comment:36 Changed 10 months ago by kallisti5

+1. We know sw_rast works. It also can't render much more than a teapot. (which is why I dropped it in favor of our llvmpipe driver)

The issue is the llvmpipe driver uses some internal hooks and private void* which have changed in recent releases. The mesa guys have been great to try and "tiptoe" around the Haiku code, but without a dedicated developer, it falls apart within a year or two.

I have a branch here working to improve the llvmpipe situation using their new API's: https://gitlab.freedesktop.org/kallisti5/mesa/commits/hgl-modernize-drawable

Currently, things crash though. Help always welcome.

Testing process:

  • git clone https://gitlab.freedesktop.org/kallisti5/mesa.git
  • git branch -u origin/hgl-modernize-drawable
  • git checkout hgl-modernize-drawable
  • scons
  • find libGL.so in the build directory and put in ~/config/non-packaged/lib/
  • mkdir ~/config/non-packaged/add-ons/opengl
  • find libswpipe.so in the build directory and put in ~/config/non-packaged/add-ons/opengl/
  • run GLInfo
  • debug :-)

comment:37 Changed 10 months ago by cocobean

@kallisti5 - Applied patches and built Mesa 18.2 (git). I reviewed the screensavers per the 'Be Advanced Topics' specs and was able to fix them to work with mesa-swpipe by doing something similar to i80and's approach. All the other GL apps I tested properly exit and kill app threads without the BeOS-specific code.

Tested with Mesa mesa-swpipe 17.1.10, 18.1.5, & 18.2 (git)

Last edited 10 months ago by cocobean (previous) (diff)

comment:38 Changed 10 months ago by cocobean

Last edited 10 months ago by cocobean (previous) (diff)

Changed 10 months ago by cocobean

Attachment: Flurry added

Flurry screensaver for Haiku hrev52153 x86_64 (fully tested/working)

comment:39 Changed 10 months ago by cocobean

Has a Patch: set

Changed 10 months ago by cocobean

Attachment: haiku-flurry.patch added

Flurry screensaver patch based on i80and patch (tested on Haiku x86_64, mesa-swpipe 17.1.10)

comment:40 Changed 10 months ago by waddlesplash

Again, please stop "testing". The problem is already well understood, and I just need to take the time to patch Mesa. i80and's approach is partially correct but not entirely so; patching Flurry to "workaround" such issues is not acceptable.

comment:41 Changed 9 months ago by waddlesplash

Milestone: UnscheduledR1/beta1
Owner: changed from kallisti5 to waddlesplash
Platform: x86-64All

comment:42 Changed 9 months ago by waddlesplash

Status: assignedin-progress

comment:43 Changed 9 months ago by waddlesplash

Resolution: fixed
Status: in-progressclosed
Note: See TracTickets for help on using tickets.