various CMake fixes and cleanup

[QuietMisdreavus]

When building swift-cmark/gfm on Windows, there are a number of issues in the CMake configuration and in our statically-generated headers that cause issues:

• The statically-generated configuration headers assume that the project is being built with Clang, and thus don't use the correct __declspec(dllexport) on functions that need to be exported.
• Also because of the above, the config header assumes that the intrinsic __builtin_expect is available, which is not the case for MSVC.
• The CMake header-include paths that were modified to support the new directory structure incorrectly modified the locations of the generated headers as well.
• The Win32 API assumes that some architecture symbols are defined that are generated in <windows.h>, which we had not properly set up as i was including <synchapi.h> directly.

Because of these and some other issues i saw while working on this, this PR applies a handful of refactors and tweaks to our build configuration:

• The statically-generated cmark-gfm_config.h now checks for the existence of the CMake-generated config.h, and includes that instead of using the statically-generated version.
• The fixes applied to cmark-gfm_export.h in build: correct export decoration macros #38 were added to cmark-gfm-extensions_export.h. However, some changes were made to fix some build issues that happened in Linux with the original change. (__attribute__((__visibility__("protected"))) turned out to cause some problems in Swift's CI.)
• Some unused portions of config.h (HAVE___ATTRIBUTE__) and *_export.h (CMARK_GFM(_EXTENSIONS)_NO_DEPRECATED) were removed.
• The HAVE_STDBOOL_H check was moved from the CMake script into a __has_include check directly in config.h. This directive is supported in GCC, Clang, and MSVC.
• <windows.h> is now included instead of <synchapi.h> on Windows. However, due to name collisions between cmark's "short" symbol names and some symbols in the Win32 API, short names are disabled universally on Windows.
• The statically-generated {project}_export.h headers have been renamed, and import the CMake-generated versions of these headers instead of using the statically-generated version. This is due to an issue in the nmake specification only building the static libraries, which shouldn't have export attributes on their methods. Since Swift will build as a dynamic library, this isn't a problem for swift build, but the change was necessary to support make builds.
• A variable in inlines.c was given an initializer, due to a warning-turned-error in an MSVC build. This has no impact on the behavior, since the variable is guaranteed to be initialized by the time it's accessed, but the static analysis failed to recognize the return statement.

With these changes, swift-cmark/gfm builds properly on Windows using nmake and by generating projects with CMake.

[QuietMisdreavus]

cc @compnerd to check my Windows work

[compnerd]

__has_include is not a standard extension, please add a fallback.

Suggested change

	#if __has_include("cmark-gfm-extensions_export.h")
	#if !defined(__has_include)
	#define __has_include(include) 0
	#endif
	#if __has_include("cmark-gfm-extensions_export.h")

[QuietMisdreavus]

Should i try to find an alternative for the section in mutex.h where it tries to find and include unistd.h?

https://github.com/apple/swift-cmark/blob/3780d733061ba47d1b9dfc4e224079e53d3f3023/src/include/mutex.h#L8-L10

That section already compiles just fine with Clang, GCC, and MSVC, in the situations that i've tried. If we're trying to avoid __has_include here, i imagine we should provide this fallback for the entire project.

[QuietMisdreavus]

It seems that __has_include was standardized in C++17 - it's valid if this is too new of a target to restrict ourselves to, but it is standard now.

[compnerd]

Yes, it is in C++17, however, the project is a C codebase, and does not follow the C++ standards but rather the C standards. I think that providing a fallback is a good idea.

[QuietMisdreavus]

I've pushed some commits that scrub out the uses of __has_include in the repo, leaving behind one in the prebuilt config.h that's behind a check.

[compnerd]

This pattern doesn't make sense to me - can you please add extensive comments on each instance for why this is needed. I'm concerned that it can result in recursive includes.

[QuietMisdreavus]

The idea behind this dance is to use the CMake-generated config.h if it's available - that's guaranteed to be applicable to the current compilation environment, as opposed to the guesses that are in the prebuilt headers used by the Swift package. The CMake-generated headers use the same header guard as the prebuilt ones - hence putting this block outside the header guard here. I've pushed a commit that tweaks how these headers are detected (as part of scrubbing out __has_include), and provides a comment explaining a little of what's going on.

[auramarua]

Apologies if I am intruding, but as you have noticed (alas, I started composing this missive before the recent commits), MSVC does not always define the __STDC_VERSION__ macro unless certain compilation options are set by using /std:c11 or later. This is partially because it is actually not a C99 compiler but a C90 compiler... with extensions.

• MSVC's language extension toggle (and how it's always on)
• MSVC's nonstandard behavior
• The /std option

By comparison __has_include is not only widely supported due to its use in modern C++ compilers (and many C compilers also being C++ compilers), including by MSVC, it will also be part of C23.

• MSVC language conformance
• WG14's N2799 on __has_include, et alia
• C Project Editor's blog on N2799's acceptance

A Godbolt demonstrating the principle at work.

I mean to invite no argument by this, mind, it is just for your bemusement.

[QuietMisdreavus]

@auramarua Right, i noticed the issue with the __STDC_VERSION__ while i was making the last batch of updates. According to the Intellisense popup, MSVC does define __STDC_VERSION__ to 199409L, which unfortunately does skip the header inclusion. (The <stdbool.h> header does exist regardless of the /std flag, though, since it was being included with the previous __has_include code.) Luckily enough, the availability of the /std flag is a bit older than the __has_include directive (VS 2017 15.0 versus VS2017 15.3), so this version of the check is that much more broad.

[compnerd]

Sorry about causing the confusion, the setting of the CMAKE_C_STANDARD and CMAKE_C_STANDARD_REQUIRED only needs to occur once (ideally soon after project(...)). That will apply to the entire project.

[QuietMisdreavus]

@daniel-grumberg @compnerd mentioned offline that he's good with this PR. You can go ahead and take a look now.

[daniel-grumberg]

This isn't a blocker, but why are we pulling in the extra stuff from windows.h since synchapi.h is a subset of that

[compnerd]

MSDN documents that you should use Windows.h and not the specific header. Additionally, there are missing declarations which are required (e.g. _AMD64_, _ARM64_ or _X86_ which are conditional).

[QuietMisdreavus]

This, basically. When we're just including <synchapi.h>, one of the headers that that includes needs the architecture-identifying symbols that are only defined in <windows.h>. That was the problem i ran into when building swift-cmark with CMake instead of SwiftPM on Windows. The best we can do is restrict which headers and symbols get included with things like WIN32_LEAN_AND_MEAN or various NO{subset} symbols.

[daniel-grumberg]

That makes sense. I didn't look into it that deeply and at the end of day including a bit more stuff than we need to really isn't a big deal since it doesn't affect the build product.