[MusicXML import] crash on Mac if file contains explicit accidental

• May 30, 2014 - 08:19
Type
Functional
Severity
S2 - Critical
Status
closed
Project
MuseScore
Tags
2.0

Importing any MusicXML file containing an explicit accidental fails, MuseScore abort with message:

ASSERT: "val >= AccidentalVal::FLAT2 && val <= AccidentalVal::SHARP2" in file /Users/lvi/dev/MuseScore/libmscore/key.h, line 93

This happens for all files with accidentals in mtest/musicxml/io:

testAccidentals1.xml
testAccidentals2.xml
testAccidentals3.xml
testGrace1.xml
testGrace2.xml
testManualBreaks.xml

Don't know when this was introduced, but when my last pull request was merged (May 22) all regression test files imported OK. Apart from the enum class changes, no changes to the MusicXML importer were made last week.

Comments

• May 31, 2014 - 13:32

This happens on OS X 10.7.5, using clang instead of gcc. Don't know yet if it is a compiler or a coding issue, but for me the assertion fails for any AccidentalVal value …

Clang version:
ls-imac:MuseScore lvi$ clang --version
Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)
Target: x86_64-apple-darwin11.4.2
Thread model: posix

A simple test program is attached, which mimics the assertion by comparing val with AccidentalVal::FLAT2 and AccidentalVal::SHARP2. Of course, if val is any legal AccidentalVal value, all comparisons should pass. This means the test should print "true" for all comparisons. Instead I get:

name SHARP2 value 2 (val >= AccidentalVal::FLAT2) false (val <= AccidentalVal::SHARP2) true
name SHARP value 1 (val >= AccidentalVal::FLAT2) false (val <= AccidentalVal::SHARP2) true
name NATURAL value 0 (val >= AccidentalVal::FLAT2) false (val <= AccidentalVal::SHARP2) true
name FLAT value -1 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) false
name FLAT2 value -2 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) false

And by simply replacing "enum class AccidentalVal" by "enum AccidentalVal" in my test program, all comparisons return true again !

Attachment Size
enum.zip 522 bytes

Reply

• May 31, 2014 - 13:40

Hmm,, OK, seems the 2 negative values throw it off track, don't understand why? ISTR lasconoc saying to use some comüpiler ption to always treat char as signed on Android, maybe this is needed here too?
Do the nightly builds have the same issue, or just your self build one?
Why not the same issue with "enum class Key : signed char"?
Does it go away if you just drop the ": signed char" or make it ": short"

Reply

• Jun 1, 2014 - 09:15

Yes, my version is self-built (OS X 10.7.5, Xcode 4.6.3, Apple LLVM version 4.2). Worked fine until one or two weeks ago.

Dropping the ": signed char" does not help. Neither does replacing the signed char by signed short or int (or even plain int).

Using an explicit typecast in the comparison ("int(val) >= int(AccidentalVal::FLAT2)" instead of "val >= AccidentalVal::FLAT2") does solve the issue.

A quick check suggest enum class Key is not used in comparisons without explicit typecast.

Tested the latest nightly (MuseScoreNightly-2014-05-31-1416-e581255.dmg): works OK.

Reply

• Jun 1, 2014 - 15:42

OK, so if the nighlies don't show the problem, on no platform, and self build doesn't show it on Windows and only on your Mac, I think the culprit is quite clear, isn't it? Now we'd need to find the exact cause, your compiler or build environment vs. the one used for the nightlies.

Reply

• Jun 2, 2014 - 17:09

The nightlies are compiled on MacOSX 10.7.5 with Apple LLVM version 4.2 (clang-425.0.24) (based on LLVM 3.2svn) (from clang -v)
XCode is not installed, but just the xcode developer tools.

xcodebuild -version gives back
Xcode 4.6
Build version 4H127

The XCode project is generated straight from the CMake files, without any patch, with cmake version 2.8.10.2 and in *release* mode (while if you are using make -f Makefile.osx xcode, it's built in debug mode).

Reply

• Jun 3, 2014 - 14:21

It could be a bug in clang, see:
http://stackoverflow.com/questions/20423108/clang-comparison-bug-with-n…

Under Mac OS X 10.8.5 and
$ clang --version
Apple LLVM version 5.1 (clang-503.0.38) (based on LLVM 3.4svn)
Target: x86_64-apple-darwin12.5.0
Thread model: posix

There seem to be no problems:
$ ./enum_clang
enum class test
name SHARP2 value 2 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) true
name SHARP value 1 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) true
name NATURAL value 0 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) true
name FLAT value -1 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) true
name FLAT2 value -2 (val >= AccidentalVal::FLAT2) true (val <= AccidentalVal::SHARP2) true

Reply

• Jun 4, 2014 - 06:21

So it seems we have a clang compiler bug that breaks the enum class comparisons and thus the assertion used in AccidentalVal. As the assertions are disabled in release builds, it does not show up in the nightly builds.

As long as no enum class comparison are used in the code (don't know for sure, did not check), this should not affect MuseScore end users. If comparisons are used, possible fixes are upgrading to LLVM version 5.1 or adding explicit typecasts.

I think either the code should be screened and fixed or an explicit note should be made not to build using LLVM version 4.2.

Reply

• Jun 4, 2014 - 08:37

Ah, yes, I didn't think of asserts being disabled in the nightlies, so thanks for solving this mystery.
I'm pretty sure enum class comparisons are used elsewhere too, but probably not in asserts.
Which means they would cause trouble to Mac users. But only of those that can be negative which restricts the possibilities by quite a lot.
Possibly further restricted to only those that compare for greater or less, but not for equal?

Example: the use of "enum class Key : signed char" in libmscore/ambitus.cpp

How difficult is a compiler upgrade?

Reply

• Jun 4, 2014 - 08:55

We kept an old version of MacOSX to build the nightlies because DMG created on Mac OSX 10.8 were not working well on Mac 10.7 (especially the background image). I believe it's the only reason. Except this, it should be easy to upgrade the build server to Mavericks and build with XCode 5 and then LLVM 5.1...

The current targeted plaform are Mac OSX 10.7+ (64 bit), just like Qt, we don't plan to support 10.6. If I'm reading correctly, with the 3 of us (Leon, ABL, myself) we can test on 10.7, 10.8 and 10.9.
So I propose to make a DMG on 10.9, and see if it works correctly. If it's ok, let's update the build server to Mavericks. Ok?

Reply

• Jun 4, 2014 - 23:17

Here is a build made on Mac OSX 10.9.3 https://www.dropbox.com/s/3zb8twyjo75mbvs/MuseScore-2.0.dmg

Can you verify that :
1/ The dmg can be opened
2/ The DMG shows icons and a background image with an down arrow pointing to the application folder
3/ Once installed, that it runs on your OS
4/ eventually if we find a case of failing comparison in release mode, that it doesn't crash?

Build env
Xcode 5.1.1
Build version 5B1008
Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn)
Target: x86_64-apple-darwin13.2.0
Thread model: posix

Reply

• Jun 5, 2014 - 10:38

I tried on 10.8.5.
1/ The dmg can be opened -> OK
2/ The DMG shows icons and a background image with an down arrow pointing to the application folder -> OK, see attachment
3/ Once installed, that it runs on your OS -> CRASH

Actually, MuseScore starts and works with "-s" option.
When sound is enabled, it crashes just after start. Attached the crash log and the console output. It seems that the libraries available in my system do not contain all the symbols present in the new version, required by libvorbis compiled in 10.9.

Attachment Size
ScreenShot.png 54.67 KB
crash_log.txt 52.77 KB
console_output.txt 411 bytes

Reply

• Jun 5, 2014 - 20:54

OS X 10.7.5:
1) the dmg opens OK
2) same image as ABL
3) runs as long as the "create new score" wizard is active, as soon as the normal score editing window is present it crashes (same "Symbol not found: ___sincos_stret" error as for ABL)

Reply

• Jun 7, 2014 - 08:44

Ok so...

If we want to build on Mac 10.9 to benefit from the new LLVM version without the bug
The dependencies (Qt, libvorbis etc...) need to be built against the lower SDK we wish to support, so 10.7. But

1/ Xcode5 doesn't come with 10.7sdk, only 10.8 and 10.9.
2/ Except Qt, we use homebrew to install dependencies and Homebrew is not really designed to build against older SDKs. See https://github.com/Homebrew/homebrew/issues/27761

The 2nd advise in this last link is what we currently follow... " In my experience though the only robust way is to build a binary for each platform or, if you can't do that, build a binary on the oldest version of OSX you wish to support (in a VM if necessary)."

Reply

• Jun 10, 2014 - 17:08

OK, that bug only seems to get trigered on comparison of signed enum class for less or greater, right?
And the only place I could find where this is done, is libmscore/key.h, line 91
So I guess we can live with some casts there, but which?
Just all, like
Q_ASSERT(int(val) >= int(AccidentalVal::FLAT2) && int(val) <= int(AccidentalVal::SHARP2));
maybe? Leon: could you try and tell us which works?

I could then just add that to my PR #945
Well, I just did, see ecb10ea

Reply

• Jun 17, 2014 - 11:16

An idea from Werner : We could reimplement the comparison operators for the signed Enums to workaround clang bug. Could be "cleaner" and more "encapsulated" than the cast in the main code.

Any thought?

Reply

• Jun 17, 2014 - 14:59

It would certainly be cleaner, but IMHO too much work for that one single case we're having a problem with, and even that only in debug mode, only on one of 3 plattforms we Support and only with an old (outdated?) Compiler (which seems unfortunatly still be needed to be able to provide MuseScore for older MacOSX Versions).
This is the only place in the code I could find where a "enum class: signed char" is compared for less or greater, which is what seems to trigger that bug.

Reply

• Jun 25, 2014 - 22:17
Status (old) active ⇒ fixed

current trunk has the casts in it. That other commit had not but did some changes to that enum
Anyway, it is fixed, so R.I.P.

Reply