Skip to content
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

SurgeXT/Linux crash on startup #4479

Closed
ghost opened this issue May 4, 2021 · 11 comments · Fixed by #4480
Closed

SurgeXT/Linux crash on startup #4479

ghost opened this issue May 4, 2021 · 11 comments · Fixed by #4480
Labels
Bug Report Item submitted using the Bug Report template
Milestone

Comments

@ghost
Copy link

ghost commented May 4, 2021

Linux 64bit
commit 52bc67e
Stop reason: signal SIGILL: illegal instruction operand
call stack (lldb):
call_stack.txt

@baconpaul
Copy link
Collaborator

Huh wow so I ran this yesterday on linux before I moved the branch. Can I ask a couple of qs

1: there were a lot of changes in the main branch. Are you sure you did a clean build? I wonder if make got confused with a force push
2: linux flavor and compiler?

but I can look this morning and see if I can repro. That stack doesn’t tell me much I’ll be honest!

thanks!

@baconpaul
Copy link
Collaborator

OK so I just tried a clean build at head and it works here:

 cmake -Bignore/linclean -DCMAKE_BUILD_TYPE=Debug
 cmake --build ignore/linclean --config Debug --target surge-xt-run-standalone --parallel 4

I also removed ~/.config/Surge XT.settings directory to get to a totally clean state

and super had a surge standalone, connected to jack, etc....

One thought: We turned on compile flags for AVX a day ago. Are you running on a CPU which is pre-2011?

@ghost
Copy link
Author

ghost commented May 4, 2021

yes it was a clean build.
linux flavor is mint 20
compilers i tried gcc 9.3.0 and clang 12.
maybe i messed something up in git, i will clone new and try again.

@ghost
Copy link
Author

ghost commented May 4, 2021

and yes the cpu is pre-2011

@baconpaul
Copy link
Collaborator

Well that will be it I’m sure. I bet gcc is adding an avx extension and that’s why you get a sigill

What a pain. Ok I’ll think about what to do today

@baconpaul
Copy link
Collaborator

Have a plan - I will make a branch for you to test today and update this issue
Thanks!!

@baconpaul
Copy link
Collaborator

OK @mynameismuhl I have an idea what to do. Can you try a branch for me and if it acts the way I think it will, then I will turn it into a mergeable branch?

So if you can here's what I'd like you to do. Basically pull and checkout my branch 'avx-test' from my baconpaul branch. This should let you build surge standalone which starts.

  1. When surge stats it will pop up a warning s saying you don't have AVX support. Dismiss that
  2. In the menu above about you will see two new items "AVX OPT NO CRASH" and "AVX FORCE CRASH"
  3. If you choose "AVX OPT NO CRASH" you should get a surge error box which says something like 'avx not supported'
  4. If you choose "AVX FORCE CRASH" you should crash immediately with a sigill

If that's what happens then I can revert the AVX build and restructure it when I add the primitives. If that's not what happens then it is a bit back to the drawing board.

If you don't do a lot of multi-remote git work here's the exact commands

git remote add baconpaul https://github.com/baconpaul/surge.git
git fetch baconpaul
git checkout baconpaul/avx-test
cmake -Bignore/avxt -DCMAKE_BUILD_TYPE=Debug
cmake --build ignore/avxt --config Debug --target surge-xt-run-standalone --parallel 4

which should get you there.

Thanks for this report! We all really appreciate the early testing you are doing here. Great stuff.

@mkruselj mkruselj added the Bug Report Item submitted using the Bug Report template label May 4, 2021
@mkruselj mkruselj added this to the Surge XT 1.0 milestone May 4, 2021
@ghost
Copy link
Author

ghost commented May 4, 2021

yes, that does exactly what you described.

@baconpaul
Copy link
Collaborator

OK fantastic. So then I know exactly how to solve it.

What I did basically was

  1. Add a new sub-library which compiled with -mavx
  2. Have that sub-library have no static members so there's no static init path
  3. Protect the primitives with CPUFeatures.hasAVX()

that stops the sigill in the startup path but still lets me swap into and out of AVX at runtime. Which is exactly what I want.

Just documenting this here since I'm going to leave a pointer to this issue in the CMakeLists when I turn off AVX-compile-time-all-units in main now.

baconpaul added a commit to baconpaul/surge that referenced this issue May 4, 2021
Blanket -mavx means that static initializers add avx intrinsics
and what not. Instead we want to use a conditional approapch through
splitting up libraries and only compiling the necessary program
units with -mavx as described in surge-synthesizer#4479.

We haven't done that yet, but we will. So even though this
Closes surge-synthesizer#4479, there's still work to be done when we use
AVX for the first time.
baconpaul added a commit that referenced this issue May 4, 2021
Blanket -mavx means that static initializers add avx intrinsics
and what not. Instead we want to use a conditional approapch through
splitting up libraries and only compiling the necessary program
units with -mavx as described in #4479.

We haven't done that yet, but we will. So even though this
Closes #4479, there's still work to be done when we use
AVX for the first time.
@baconpaul
Copy link
Collaborator

OK I merged back to main so you should start without a sigill now

its possible in the xt cycle that we use AVX to improve the performance of one or two spots in the code but we can keep the non-avx branches alive

the about screen should show you something like '(no AVX)' next to your processor name too.

@ghost
Copy link
Author

ghost commented May 4, 2021

great. works. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Report Item submitted using the Bug Report template
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants