Avoid includes within `extern C` namespace

carlopi commented

2024-07-31 07:43:05 -05:00

(Migrated from github.com)

Line 244 of xxhash.h has a:

#if defined (__cplusplus)
extern "C" {
#endif

that is followed up by includes of system-provided headers, that are not guaranteed to work within an extern C linkage rules.

In particular, while compiling apache-arrow with Emscripten, I get:

/Users/carlo/emsdk/upstream/emscripten/cache/sysroot/include/emscripten/em_asm.h:151:1: error: templates must have C++ linkage
  151 | template<typename, typename = void> struct __em_asm_sig {};
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/vendored/xxhash/xxhash.h:173:1: note: extern "C" language linkage specification begins here
  173 | extern "C" {
      | ^
In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/compute/kernels/vector_hash.cc:27:
In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/array/dict_internal.h:37:
In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/util/hashing.h:49:
In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/vendored/xxhash.h:18:
In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/vendored/xxhash/xxhash.h:3434:
In file included from /Users/carlo/emsdk/upstream/emscripten/cache/sysroot/include/compat/arm_neon.h:293:
In file included from /Users/carlo/emsdk/upstream/emscripten/cache/sysroot/include/emscripten.h:1:

that make compilation fail.

Current workaround, very much ad-hoc, is explicitly including <emscripten.h> (conditionally on EMSCRIPTEN being defined) before including xxhash.h. This works, but it's not great and solves only this specific case. PR is at https://github.com/duckdb/duckdb-wasm/pull/1803.

Would it be possible to move the includes outside of the extern C namespace, given I believe not to be compliant with C++ compilers?

Thanks a lot.

[Line 244](https://github.com/Cyan4973/xxHash/blob/dev/xxhash.h#L244) of xxhash.h has a: ```c++ #if defined (__cplusplus) extern "C" { #endif ``` that is followed up by includes of system-provided headers, that are not guaranteed to work within an `extern C` linkage rules. In particular, while compiling apache-arrow with Emscripten, I get: ``` /Users/carlo/emsdk/upstream/emscripten/cache/sysroot/include/emscripten/em_asm.h:151:1: error: templates must have C++ linkage 151 | template<typename, typename = void> struct __em_asm_sig {}; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/vendored/xxhash/xxhash.h:173:1: note: extern "C" language linkage specification begins here 173 | extern "C" { | ^ In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/compute/kernels/vector_hash.cc:27: In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/array/dict_internal.h:37: In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/util/hashing.h:49: In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/vendored/xxhash.h:18: In file included from /Users/carlo/duckdb-wasm/submodules/arrow/cpp/src/arrow/vendored/xxhash/xxhash.h:3434: In file included from /Users/carlo/emsdk/upstream/emscripten/cache/sysroot/include/compat/arm_neon.h:293: In file included from /Users/carlo/emsdk/upstream/emscripten/cache/sysroot/include/emscripten.h:1: ``` that make compilation fail. Current workaround, very much ad-hoc, is explicitly including `<emscripten.h>` (conditionally on EMSCRIPTEN being defined) before including `xxhash.h`. This works, but it's not great and solves only this specific case. PR is at https://github.com/duckdb/duckdb-wasm/pull/1803. Would it be possible to move the includes outside of the `extern C` namespace, given I believe not to be compliant with C++ compilers? Thanks a lot.

👍 1

Cyan4973 commented

2024-07-31 10:26:58 -05:00

(Migrated from github.com)

possibly ?
The "generic" headers (like <stddef.h>) can most likely be pushed outside,
but the "specific" ones (like <emscriptem.h>) are only included if some specific conditions are met, and it would be necessary to replicate these conditions outside of extern "C" block, which now becomes must less desirable for code maintenance.

To be investigated.

possibly ? The "generic" headers (like `<stddef.h>`) can most likely be pushed outside, but the "specific" ones (like `<emscriptem.h>`) are only included if some specific conditions are met, and it would be necessary to replicate these conditions outside of `extern "C" ` block, which now becomes must less desirable for code maintenance. To be investigated.

Cyan4973 commented

2024-07-31 11:18:59 -05:00

(Migrated from github.com)

What would make sense is to fold this objective into the planned effort to separate the source code into multiple small units.
There it will be easier to pay attention to #include outside of the extern "C" block

What would make sense is to fold this objective into the planned effort to separate the source code into multiple small units. There it will be easier to pay attention to `#include` outside of the `extern "C"` block

carlopi commented

2024-07-31 15:45:20 -05:00

(Migrated from github.com)

Thanks for the reply!
Possibly a way out, altought very verbose, is closing and then reopening the extern C brackets, something like:

#if defined(__cplusplus)
extern `C`
#endif

// some code

#if something
#include <x>
#endif

// some more code

#if defined(__cplusplus)
}
#endif

to

#if defined(__cplusplus)
extern `C`
#endif

// some code

#if defined(__cplusplus)
}
#endif

#if something
#include <x>
#endif

#if define(__cplusplus)
extern `C`
#endif

// some more code

#if defined(__cplusplus)
}
#endif

unsure if doing this via macro or something would make sense.

Thanks for the reply! Possibly a way out, altought very verbose, is closing and then reopening the `extern C` brackets, something like: ```c #if defined(__cplusplus) extern `C` #endif // some code #if something #include <x> #endif // some more code #if defined(__cplusplus) } #endif ``` to ```c #if defined(__cplusplus) extern `C` #endif // some code #if defined(__cplusplus) } #endif #if something #include <x> #endif #if define(__cplusplus) extern `C` #endif // some more code #if defined(__cplusplus) } #endif ``` unsure if doing this via macro or something would make sense.

Cyan4973 commented

2024-07-31 16:00:15 -05:00

(Migrated from github.com)

There are limits (one cannot generate macros with macros), but yeah, something like that is indeed possible.
It just will fit more naturally in a multi-units design, from which a single amalgamated xxhash.h could still be generated.

There are limits (one cannot generate macros with macros), but yeah, something like that is indeed possible. It just will fit more naturally in a multi-units design, from which a single amalgamated `xxhash.h` could still be generated.

👍 2

t-mat commented

2024-08-01 06:10:24 -05:00

(Migrated from github.com)

As another possible solution, we'll be able to use the following inline extern C style

extern "C" void my_public_func(void);

We can also define it as a macro

#if defined(__cplusplus)
#define MY_PUBLIC_API extern "C"
#endif

MY_PUBLIC_API void my_public_func(void);

Msvc's dllexport is able to be supported in the same style.

As another possible solution, we'll be able to use the following inline extern C style ``` extern "C" void my_public_func(void); ``` We can also define it as a macro ``` #if defined(__cplusplus) #define MY_PUBLIC_API extern "C" #endif MY_PUBLIC_API void my_public_func(void); ``` Msvc's dllexport is able to be supported in the same style.

👍 2

Cyan4973 commented

2024-12-27 20:28:50 -06:00

(Migrated from github.com)

I was wondering if there was some kind of relatively easy scenario that could be played locally in order to produce this issue ?

carlopi commented

2024-12-28 03:58:00 -06:00

(Migrated from github.com)

It's not super trivial, I have not been able to reproduce locally now, possibly this has been fixed in Emscripten headers (I tried 3.1.71 and 3.1.61).

I would say it's not safe to include headers that might be implemented arbitrarily within extern C namespaces, since those headers might have branches on cplusplus being there for feature detection or so.

But given I can't provide a reproduction AND this might be very minor, your call on whether this makes sense looking into or not.

It's not super trivial, I have not been able to reproduce locally now, possibly this has been fixed in Emscripten headers (I tried 3.1.71 and 3.1.61). I would say it's not safe to include headers that might be implemented arbitrarily within `extern C` namespaces, since those headers might have branches on cplusplus being there for feature detection or so. But given I can't provide a reproduction AND this might be very minor, your call on whether this makes sense looking into or not.

Cyan4973 commented

2024-12-29 02:01:06 -06:00

(Migrated from github.com)

I understand that it's generally preferable to avoid using #include "..." statements within an extern "C" block, and I would prefer to eliminate such cases.

The challenge, however, lies in the current structure of the code. Attempting this change would be a significant undertaking, deeply affecting the source code and potentially introducing new risks in the process.

Additionally, without any testing framework in place, there's no way to confirm that the change would actually address any tangible issue. This introduces considerable uncertainty, as the benefits are unclear and it's difficult to identify or reproduce scenarios where this modification would make a meaningful difference.

Of course, if the change were straightforward, it would be an entirely different matter, and I would simply implement it. I believe that, in the future, as the source code is refactored into smaller, more manageable units, this goal will naturally become more feasible and much easier to achieve.

I understand that it's generally preferable to avoid using `#include "..."` statements within an `extern "C"` block, and I would prefer to eliminate such cases. The challenge, however, lies in the current structure of the code. Attempting this change would be a significant undertaking, deeply affecting the source code and potentially introducing new risks in the process. Additionally, without any testing framework in place, there's no way to confirm that the change would actually address any tangible issue. This introduces considerable uncertainty, as the benefits are unclear and it's difficult to identify or reproduce scenarios where this modification would make a meaningful difference. Of course, if the change were straightforward, it would be an entirely different matter, and I would simply implement it. I believe that, in the future, as the source code is refactored into smaller, more manageable units, this goal will naturally become more feasible and much easier to achieve.

carlopi commented

2024-12-29 03:06:32 -06:00

(Migrated from github.com)

Agree, then probably closing this, if this pops back for some other weird libc implementation then maybe there will be a better shot at reproducing.

👍 1

BinBashBanana commented

2025-10-13 20:40:17 -05:00

(Migrated from github.com)

I ran into this issue but I was able to solve it by adding this at the top of xxhash.h:

#ifdef __wasm_simd128__
#include <arm_neon.h>
#endif

I ran into this issue but I was able to solve it by adding this at the top of `xxhash.h`: ```cpp #ifdef __wasm_simd128__ #include <arm_neon.h> #endif ```

👍 1

Cyan4973 commented

2026-09-14 17:44:06 -05:00

(Migrated from github.com)

This issue is presumed fixed in #1129

Rows
Columns

Avoid includes within extern C namespace #960

Avoid includes within `extern C` namespace #960