View Issue Details

IDProjectCategoryView StatusLast Update
0001280OpenMPT[All Projects] libopenmptpublic2019-11-10 17:47
Reportermanx Assigned Tomanx  
PriorityhighSeveritymajorReproducibilityalways
Status assignedResolutionopen 
Product VersionOpenMPT 1.29.00.* (current testing) 
Target VersionOpenMPT 1.28.09.00 / libopenmpt 0.4.11 (upcoming stable)Fixed in Version 
Summary0001280: ban function-static data
Description

We must ban function-local static data.

static data inside of a function must use a lock to be initialized thread-safely by the compiler. This is required in C++11 and implemented even for C++03 by gcc and clang. The compiler is optionally allowed to use constant-initialization instead, but is by no means and in no circumstances required to do so (not even when the data is declared const and not even if the initializing expression is a constant expression). Current MSVC doesn't use constant-initialization in debug builds and thus introduces a lock, and older MSVC just silently introduces a race condition because it does not implement thread-safe initialization.

static const data inside a function is a bug, in both, C++ pre 11, and post 11.

This affects all active branches:

  • libopenmpt 0.2 (C++03): not thread-safe with GCC 4.2 or any MSVC except VS2015 targeting Windows 7
  • libopenmpt 0.3 (C++11): not thread-safe with any MSVC in Windows XP builds (compiler requires Vista for thread-safe init)
  • libopenmpt 0.4 (C++11): not thread-safe with any MSVC in Windows XP builds (compiler requires Vista for thread-safe init)
  • libopenmpt 0.5 (C++17): sub-optimal code generation due to possibility of compiler-induced locking

There are 3 alternatives:

  • constexpr (with or without static, but without static is much cleaner and easier to read, because static induces thinking about initialization when reading the code) for c++17 or later (possibly also for c++11, however constexpr restrictions probably make this solution infeasible)
  • const data, the compiler is likely to initialize it at compile time anyway, losing nothing compared to the current solution which also does not guarantee anything. this solution requires extra caution when returning a pointer to the data, which would be dangling unless the pointed-to data is a string literal.
  • namespace scope static data, however this is not guaranteed to not generate run-time global initialization without using constexpr either.

Note: function-local static data is no bug in C, because C requires initializers to be constant expressions.

TagsNo tags attached.
Has the bug occurred in previous versions?
Tested code revision (in case you know it)

Activities

Saga Musix

Saga Musix

2019-11-10 17:47

administrator   ~0004149

For static LUTs in module loaders, typically for pattern command conversion, I prefer them to be close to their usage site, so constexpr initialization seems to be the best candidate there. Otherwise putting them at the top of the file with constexpr would be fine too. Apart from a few static const variables here and there (which could also be trivially turned into constexpr), I guess those would be the biggest offenders in soundlib/?

Issue History

Date Modified Username Field Change
2019-11-06 16:02 manx New Issue
2019-11-06 16:02 manx Status new => assigned
2019-11-06 16:02 manx Assigned To => manx
2019-11-10 17:47 Saga Musix Note Added: 0004149