Fix compiler errors in GVN.h for clang versions older than 15

[fsfod]

Fix compiler errors in GVN.h from clang versions older than 15 that don't support gnu style attribute on namespaces. @tstellar, @compnerd.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-support

Author: Thomas Fransham (fsfod)

Changes

Fix compiler errors in GVN.h from clang versions older than 15 that don't support gnu style attribute on namespaces. @tstellar, @compnerd.

---

Full diff: https://github.com/llvm/llvm-project/pull/107636.diff

1 Files Affected:

• (modified) llvm/include/llvm/Support/Compiler.h (+5)

diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index e1d54af7dfcce1..0452f88cbf2c66 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -130,7 +130,12 @@
 
 #if (!(defined(_WIN32) || defined(__CYGWIN__)) ||                              \
      (defined(__MINGW32__) && defined(__clang__)))
+// Clang compilers older then 15 do not support gnu style attributes on namespaces
+#if defined(__clang__) && _clang_major__ < 15
+#define LLVM_LIBRARY_VISIBILITY [[gnu::visibility("hidden")]]
+#else
 #define LLVM_LIBRARY_VISIBILITY LLVM_ATTRIBUTE_VISIBILITY_HIDDEN
+#endif
 #define LLVM_ALWAYS_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #elif defined(_WIN32)
 #define LLVM_ALWAYS_EXPORT __declspec(dllexport)

[mstorsjo]

Typo, you have one leading underscore on __clang_major__ while you should have two.

Note that with Apple Clang, there's a separate different versioning, where __clang_major__ reflects the Apple versioning. If __apple_build_version__ is defined, then __clang_major__ reflects the Xcode version number, distinct from the upstream LLVM.org version numbers.

According to https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)_2, Xcode 14.0 - 14.2 maps to LLVM 14.0, while Xcode 14.3 maps to LLVM 15.0.

So out of pure luck and coincidence, the cutoff we're looking at might actually mostly work out here with one single version number check :-)

[fsfod]

I guess i should of read further down when i went looking for the correct macro on this page https://github.com/cpredef/predef/blob/master/Compilers.md#clang

[mstorsjo]

Unfortunately, it seems like this isn't enough to fix the build issues:

In file included from /Users/martin/code/llvm-project/llvm/lib/Support/BLAKE3/blake3_portable.c:1:
/Users/martin/code/llvm-project/llvm/lib/Support/BLAKE3/blake3_impl.h:186:1: error: expected expression
LLVM_LIBRARY_VISIBILITY
^
/Users/martin/code/llvm-project/llvm/include/llvm/Support/Compiler.h:136:34: note: expanded from macro 'LLVM_LIBRARY_VISIBILITY'
#define LLVM_LIBRARY_VISIBILITY [[gnu::visibility("hidden")]]
                                 ^
In file included from /Users/martin/code/llvm-project/llvm/lib/Support/BLAKE3/blake3_portable.c:1:
/Users/martin/code/llvm-project/llvm/lib/Support/BLAKE3/blake3_impl.h:187:1: error: expected identifier or '('
void blake3_compress_in_place(uint32_t cv[8],

[mstorsjo]

At this point, I think it would be safest to revert, reevaluate and test the known problematic cases before trying to reland this.

[fsfod]

is that on a old version of clang as well?

[mstorsjo]

is that on a old version of clang as well?

Yes, this was on Apple Clang 12, which should correspond to llvm.org Clang 10-11.

[mstorsjo]

And just to be clear - the issue here is that LLVM_LIBRARY_VISIBILITY now expands to a C++ attribute, but we're using it in a couple of pure C files, which don't support C++ style attributes.

[fsfod]

I guess i will have to change my LLVM_C_ABI macro as well although my changes to switch that file to use are not in yet.

The alternative fix i had before was just a separate macro for namespaces, I was just trying to avoid a proliferation of visibility macros for different code elements.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 7f77db4ffca9b275b40e8720208a03dd6cbc390e 1ee44f3d3ab976b35b1e81e2fe91da86684d8534 --extensions h -- llvm/include/llvm/Support/Compiler.h llvm/include/llvm/Transforms/Scalar/GVN.h

View the diff from clang-format here.

diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h
index be6c0ec5ed..e6adb183ff 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVN.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVN.h
@@ -62,7 +62,7 @@ struct AvailableValue;
 struct AvailableValueInBlock;
 class GVNLegacyPass;
 
-} // end namespace gvn
+} // namespace LLVM_LIBRARY_VISIBILITY_NAMESPACE gvn
 
 /// A set of parameters to control various transforms performed by GVN pass.
 //  Each of the optional boolean parameters can be set to:

[mstorsjo]

LGTM, this seems to fix my build on Apple Clang 12 (at least it seems to not fail for quite a while).

[michaelrj-google]

Please either revert the original commit or land this commit to fix the issue. The libc runtimes builder is currently broken on this issue since it is on clang 14: https://lab.llvm.org/buildbot/#/builders/78/builds/5463

[fsfod]

You can commit it for me if for if you want since i don't have commit access.