Support older versions of GCC

[iskunk]

libsoundio-older-gcc.patch.txt

This patch against the Git v2 branch contains the following:

• New implementation of SOUNDIO_ATOMIC_* based on GCC's __sync_*() builtins
• Better feature-macro test logic in atomics.h to select the appropriate implementation of SOUNDIO_ATOMIC_*
• CMake logic to determine support for -std=c11, as well as some warning flags that older GCC versions don't know about

[ligfx]

Looks like this is a follow-up to #111.

Couple things:

• Why test for CMAKE_COMPILER_IS_GNUCC? This ignores Clang and other non-GNU compilers with compatible flags. check_c_compiler_flag will figure out what works and what doesn't anyways.

• Each of EXTRA_DEBUG_CFLAGS and EXTRA_WARNING_CFLAGS should be checked and added separately, so any compiler without support for a specific warning will still benefit from the others. I've seen other projects do something like:

  function(check_and_add_flag var flag)
      check_c_compiler_flag("${flag}" _flag_check)
      if(_flag_check)
          set(${var} "${${var}} ${flag}" PARENT_SCOPE)
      endif()
  endfunction()
  
  check_and_add_flag(CMAKE_C_FLAGS_DEBUG "-Werror")
  check_and_add_flag(CMAKE_C_FLAGS_DEBUG "-pedantic")
  check_and_add_flag(LIB_CFLAGS "-Werror=strict-prototypes")
  check_and_add_flag(LIB_CFLAGS "-Werror=old-style-definition")
  check_and_add_flag(LIB_CFLAGS "-Werror=missing-prototypes")
  check_and_add_flag(LIB_CFLAGS "-Wno-missing-braces")

• Why the additional restriction on using C++11 atomics (__cplusplus >= 201103L || (defined(_MSC_VER) && _MSC_VER >= 1700))?

• Instead of checking __STDC_VERSION__ to known values to infer the presence of stdatomic.h, CMake can check for the header file directly with check_include_file.

• Similarly, CMake can directly check for the availability of __sync_fetch_and_add, __sync_synchronize, and __sync_val_compare_and_swap.

• The "braced-groups within expressions" and pragma stuff to avoid -Wunused-value warnings seems heavy-handed to me. Isn't the typical way to accomplish this by prefixing expressions with (void)?

• I'd really recommend using Pull Requests. They help you get feedback faster by virtue of being easier for others to read, comment on, and test locally, and they also solve the problem of people continually commenting "Please use PRs" ;)

[iskunk]

Hi @ligfx!

Re CMAKE_COMPILER_IS_GNUCC: Yes, I was in fact about to upload a revised patch as I had encountered this issue while test-building on newer FreeBSD, and on Linux with Intel's ICC. Having build scripts set compiler flags directly is a bit dodgy, so you have to be sure that the compiler is what you think it is.

Re each of EXTRA_*_CFLAGS: Yes, testing each flag individually would be better inasmuch as you get the actual subset that is supported. The reason why I didn't do that is because there would be little benefit (if you're building with an older compiler, then you're probably not doing development, so warnings are superfluous anyway), and @andrewrk already hasn't shown the greatest enthusiasm for supporting older systems, so I wanted to keep the size of this change under control.

Re C++11 atomic restriction: You can't just assume that because you're using a C++ compiler, that C++11 atomics are available. Visual Studio didn't have them until 11.0 (VS2012).

Re __STDC_VERSION__: The stdatomic.h header may throw an error if C11 mode is not enabled. (You might want to compile in non-C11 mode even when you do have the header, like if you want to test the GCC-atomic implementation.)

Re __sync_* detection: The set of __sync_* builtins has remained the same since GCC 4.1, so there isn't really much to detect beyond the compiler + version.

Re -Wunused-value warnings: The return value isn't always unused :-)

Re pull requests: You don't think patches are easier to read? That's the pure change, boiled down. Testing locally, ehh... applying a patch versus cloning/checking out the appropriate commit... bit of a wash, seems like...

[iskunk]

Here is my revised patch:

libsoundio-older-gcc-v2.patch.txt

This also addresses a couple of wrinkles found in testing: The math functions sometimes (but not always) require -lm, and clock_gettime() sometimes (but not always) requires -lrt. Clang as well as Intel ICC are now handled correctly, and I fixed some quoting issues with CMake (empty variables are the same as no argument at all, not unlike a shell, so quotes are needed to hold their place).

By the way, on a related note... is SoundIoAtomicLong intended to be a 64-bit type? Because long is normally the same size as int, even on 64-bit systems...

[ligfx]

struct SoundIoAtomicBool {
    bool x;
    bool is_bool;
};
// Note: Only works for SoundIoAtomicBool
#define SOUNDIO_ATOMIC_EXCHANGE(a, value) \
    ({ bool _value = value; \
       (void)a.is_bool; \
       __sync_val_compare_and_swap(&a.x, !_value, _value); })

Simpler and more semantic to use the compiler's built-in type-checking:

struct SoundIoAtomicBool {
    bool x;
};
__attribute__((always_inline))
inline bool SOUNDIO_ATOMIC_EXCHANGE(SoundIoAtomicBool a, bool value) {
    return __sync_val_compare_and_swap(&a.x, !value, value);
}

Re -Wunused-value warnings: The return value isn't always unused :-)

According to the type signatures of the other implementations:

• SOUNDIO_ATOMIC_FETCH_ADD should have a return value.
• SOUNDIO_ATOMIC_STORE should never have a return value.
• SOUNDIO_ATOMIC_EXCHANGE should have a return value.
• SOUNDIO_ATOMIC_FLAG_TEST_AND_SET should have a return value.

Re CMAKE_COMPILER_IS_GNUCC: Yes, I was in fact about to upload a revised patch as I had encountered this issue while test-building on newer FreeBSD, and on Linux with Intel's ICC. Having build scripts set compiler flags directly is a bit dodgy, so you have to be sure that the compiler is what you think it is.

The new patch is still effectively removing build features while making it more complicated. Now, only Clang|GNU|Intel compilers get the chance to benefit from certain flags, while other logic is duplicated between "Clang|GNU|Intel" and others.

My point above was that, since you're checking whether the flags work anyways, you don't and shouldn't check the compiler IDs. You've replaced a simple, albeit not-always-true, knowledge check ("all non-MSVC compilers support these flags"), with a redundant combination of a knowledge check ("compilers with these IDs might support these flags") and an empirical test ("does the current compiler support these flags?").

All other things equal, it's better to rely on empirical testing, which will always work, than on knowledge checks, which can become outdated or be too general. (Main overriding concerns I've seen include 1) testing for MSVC specifically, since it's so different, and 2) simple knowledge checks, as the current state of the code, where simplicity and getting started are more important than working everywhere).

Re each of EXTRA_*_CFLAGS: Yes, testing each flag individually would be better inasmuch as you get the actual subset that is supported. The reason why I didn't do that is because there would be little benefit (if you're building with an older compiler, then you're probably not doing development, so warnings are superfluous anyway), and @andrewrk already hasn't shown the greatest enthusiasm for supporting older systems, so I wanted to keep the size of this change under control.

I'm thinking of a case with either a current uncommon compiler, or a future compiler, where certain flags don't work but others do. Not sure how likely that is.

I'm still a fan of separating flags out, since I think it's easier to keep a handle on mentally, and can be done in a very semantic, declarative way ("We want these flags, if possible").

Re C++11 atomic restriction: You can't just assume that because you're using a C++ compiler, that C++11 atomics are available. Visual Studio didn't have them until 11.0 (VS2012).

Okay, this isn't related to "Support older versions of GCC," and should go in a separate patch.

Re STDC_VERSION: The stdatomic.h header may throw an error if C11 mode is not enabled. (You might want to compile in non-C11 mode even when you do have the header, like if you want to test the GCC-atomic implementation.)

Right, CMake checks that a header is 1) includable and 2) compilable. check_include_file("stdatomic.h" HAVE_STDATOMIC) would do the trick.

Re pull requests: You don't think patches are easier to read? That's the pure change, boiled down. Testing locally, ehh... applying a patch versus cloning/checking out the appropriate commit... bit of a wash, seems like...

100% Pull Requests are easier to deal with than patches. They're integrated with the rest of GitHub, so you can comment inline on specific changes, see the context in the rest of the code, see the difference between revisions, and link them to from elsewhere.

It comes down to consistency: when everything else is using Pull Requests, it's a break in workflow to download and look at a patch. This involves more time and effort, making it less likely for your patch to get reviewed.

[iskunk]

According to the type signatures of the other implementations:

Yep, all that's covered.

My point above was that, since you're checking whether the flags work anyways, you don't and shouldn't check the compiler IDs. You've replaced a simple, albeit not-always-true, knowledge check ("all non-MSVC compilers support these flags"), with a redundant combination of a knowledge check ("compilers with these IDs might support these flags") and an empirical test ("does the current compiler support these flags?")

What you know is that compilers with these IDs can take options that look like -Wfoo and -ffoo, but not necessarily every possible flag along those lines.

It's quite risky to test a set of flags against a compiler without knowing anything about it, because sometimes the results play out unpredictably. Here's a tidbit from one of my other projects where I had to deal with this (note the bit about -mthreads):

# -mt: Sun Workshop C (may only link SunOS threads [-lthread], but it
#      doesn't hurt to check since this sometimes defines pthreads and
#      -D_REENTRANT too), HP C (must be checked before -lpthread, which
#      is present but should not be used directly; and before -mthreads,
#      because the compiler interprets this as "-mt" + "-hreads")

All other things equal, it's better to rely on empirical testing, which will always work

It's a lot harder than you're giving it credit for. (And my work on AX_PTHREAD has been hampered by no longer having access to some of the systems it supports for testing.)

Main overriding concerns I've seen include 1) testing for MSVC specifically, since it's so different

You can thank Microsoft for that ^_^

(Besides, as I understand, the C++ implementation was added primarily for the benefit of Windows builds)

simple knowledge checks, as the current state of the code, where simplicity and getting started are more important than working everywhere

Once your project specifies a set of compiler flags that augments/overrides the user's, you've left the path of simplicity. In normal practice, you're supposed to use whatever flags the user specifies, and if those aren't good enough, then complain appropriately.

I'm thinking of a case with either a current uncommon compiler, or a future compiler, where certain flags don't work but others do. Not sure how likely that is.

Well, there's many other compilers besides Clang/GNU/Intel. That's why I added a third clause to the big flag-conditional, the one that only sets -D flags. It's a sign of code-amateurism when you try to build a project on (say) AIX and xlc_r errors out because it was passed -Wall.

I'm still a fan of separating flags out, since I think it's easier to keep a handle on mentally, and can be done in a very semantic, declarative way ("We want these flags, if possible").

Yes, that is a good approach, though you still have to limit the set of flags by compiler category (Clang, GNU) or system (non-Clang/GCC on AIX, Solaris, etc.). I'd do (and have done) that kind of thing. But here, it's not really worth the candle. What's the benefit? All that matters is that the build doesn't break.

Okay, this isn't related to "Support older versions of GCC," and should go in a separate patch.

That's one of the nice things about patches: A maintainer can break them out however s/he likes across multiple commits, rather than me having to guess what level of granularity is desired.

Right, CMake checks that a header is 1) includable and 2) compilable. check_include_file("stdatomic.h" HAVE_STDATOMIC) would do the trick.

Don't forget the bit in config.h, too.

Sure, that approach would work. But then you have to touch three files instead of one, and the conditional no longer changes when you switch between -std=c11 and -std=c99. Are you really any better off?

It comes down to consistency: when everything else is using Pull Requests, it's a break in workflow to download and look at a patch. This involves more time and effort, making it less likely for your patch to get reviewed.

What happens when there are changes in someone else's tree that you want, but you want them in a different form? Maybe you want to structure the commits differently, maybe the code is indented a little sloppily in places. Accepting a pull request means that whatever the developer is submitting is going into your repo as-is.

[mdsitton]

Either you fix the issues in a post merge commit or ask the submitter to fix the issues and to squash unneeded commits and restructuring the pull request.

[ligfx]

According to the type signatures of the other implementations:

Yep, all that's covered.

Then why the braced-groups within expressions and Superfluous ({...}) are to quell -Wunused-value warnings? I'm missing something here.

Well, there's many other compilers besides Clang/GNU/Intel. That's why I added a third clause to the big flag-conditional, the one that only sets -D flags. It's a sign of code-amateurism when you try to build a project on (say) AIX and xlc_r errors out because it was passed -Wall.

It wouldn't, because -Wall will be checked by check_c_compiler_flag. Please try to avoid ad hominems like "code-amateurism".

Okay, this isn't related to "Support older versions of GCC," and should go in a separate patch.

That's one of the nice things about patches: A maintainer can break them out however s/he likes across multiple commits, rather than me having to guess what level of granularity is desired.

The issue title should be renamed "Support older versions of GCC and other compilers," then.

Generally it's nice to make the maintainer do as little work as possible... Instead of complaining, why not just split the patch out? I've found that code gets accepted much faster when I do what people ask in good faith, rather than argue technicalities.

Sure, that approach would work. But then you have to touch three files instead of one, and the conditional no longer changes when you switch between -std=c11 and -std=c99. Are you really any better off?

On macOS, stdatomic.h still works when -std=c99 is passed on the command line. Are there any major compiler versions that don't support -std=c11 but do have stdatomic.h?

[iskunk]

Either you fix the issues in a post merge commit or ask the submitter to fix the issues and to squash unneeded commits and restructuring the pull request.

A merge won't restructure the commits. Unneeded commits is one possibility; not enough separate commits (or even separate pulls) is another. Asking a submitter to restructure their submission... it's like... it's not enough that they're giving you fifty quid, but they have to give you exactly three tens and four fives? Completely apart from the content of the change, which is what matters at the end of the day.

I mean, I've done pull requests before, but it wears down the patience of both the maintainer and the submitter when you have issues not related to the actual changes getting in the way. From my perspective, patches lower the stakes. It's "just gimme the goods, screw the formalities."

Then why the braced-groups within expressions and Superfluous ({...}) are to quell -Wunused-value warnings? I'm missing something here.

The braced-groups are needed for the EXCHANGE and STORE operations, to group together the multiple statements into one. For FETCH_ADD and TEST_AND_SET, they are not strictly necessary, but quash some -Wunused-value warnings as a side effect when those ops are used without looking at the return value.

(Was that what you were looking for? I'm not quite sure what you were asking...)

It wouldn't, because -Wall will be checked by check_c_compiler_flag.

Unless compiler FooCC interprets that as -Wa -ll, and there is a libl somewhere, and the check succeeds. You can't assume.

The only flags that are generally safe across the board are the bog-standard ones: -D<def> -U<def> -I<dir> -L<dir> -l<lib> -O -g -o, probably a couple more. And even those are a bit iffy when you're talking Windows compilers that want their flags to start with a forward-slash.

Please try to avoid ad hominems like "code-amateurism".

That's not an ad hominem; that's an ad codinum ^_^

More seriously, of course people don't do this because there's something wrong with them. People do it because, when all they've ever used is Clang/GCC, and those do everything they need to do, why seek out anything beyond? It's not a problem as long as they're willing to allow support for farther-flung systems when the point arises.

Remember, too: "Amateur" is not an insult. (Unless applied to a professional software developer...)

The issue title should be renamed "Support older versions of GCC and other compilers," then.

Negative; the __sync_*() builtins are only for GCC and its equivalents. Issue 111 had what you say as its goal, but you saw how that went.

Incidentally, there are some 3rd-party libraries out there that provide cross-platform atomic operations (Concurrency Kit and libatomic-ops), but even I don't care enough to bother with them.

Generally it's nice to make the maintainer do as little work as possible... Instead of complaining, why not just split the patch out? I've found that code gets accepted much faster when I do what people ask in good faith, rather than argue technicalities.

I'll do that if a maintainer asks. But it's not necessarily the case that splitting out a patch makes less work for him/her; we're just guessing. What I often do, for example, is dump a bunch of changes into my tree, test them out, and then put together one or more commits just the way I want them by firing up git gui and making liberal use of "Stage {Hunk,Lines} For Commit." More patches just means more startup overhead (especially if they conflict).

On macOS, stdatomic.h still works when -std=c99 is passed on the command line. Are there any major compiler versions that don't support -std=c11 but do have stdatomic.h?

Well, facilely, some non-GNU/Clang compiler that is C11 compliant but takes a different flag to enable that mode. But even then, it's a lot to assume that a C11 header will work correctly if you're not in C11 mode.

[ligfx]

For FETCH_ADD and TEST_AND_SET, they are not strictly necessary, but quash some -Wunused-value warnings as a side effect when those ops are used without looking at the return value.

Oh, I see. The <stdatomic.h> functions have return values marked as possibly unused, so you're imitating the API exactly. That's weird that they do that. I wish there was a better way.

Did you see my earlier comment about type-checking the EXCHANGE operation?

Unless compiler FooCC interprets that as -Wa -ll, and there is a libl somewhere, and the check succeeds. You can't assume.

The only flags that are generally safe across the board are the bog-standard ones: -D -U -I

-L -l -O -g -o, probably a couple more. And even those are a bit iffy when you're talking Windows compilers that want their flags to start with a forward-slash.

We're getting off-topic. It's highly unlikely this project will ever be compiled by a FooCC with pathological command-line behaviour.

The issue title should be renamed "Support older versions of GCC and other compilers," then.

Negative; the _sync*() builtins are only for GCC and its equivalents. Issue 111 had what you say as its goal, but you saw how that went.

I meant that since this also affects the C++ implementation, and changes the behavior of CFLAGS, then "Support older versions of GCC" no longer accurately describes the entire patch.

[iskunk]

Oh, I see. The <stdatomic.h> functions have return values marked as possibly unused, so you're imitating the API exactly. That's weird that they do that. I wish there was a better way.

I wasn't trying to imitate stdatomic.h; the only API of concern here is SOUNDIO_ATOMIC_*(). It's just that libsoundio in a couple of places does FETCH_ADD and TEST_AND_SET without using the return value, and those were giving spurious warnings.

Did you see my earlier comment about type-checking the EXCHANGE operation?

Ah, missed that. Yes, an inline function would work handily there :-)

We're getting off-topic. It's highly unlikely this project will ever be compiled by a FooCC with pathological command-line behaviour.

Until it is. If libsoundio becomes popular---and I'm certainly hoping to see that---the probability will approach 1.

I meant that since this also affects the C++ implementation, and changes the behavior of CFLAGS, then "Support older versions of GCC" no longer accurately describes the entire patch.

Ensuring correct feature-test conditional behavior and proper compiler and compatible-flags identification are part and parcel of this work. What different title would you suggest?