[compiler-rt] [emutls] Handle unused parameters in a compiler agnostic way

[mstorsjo]

Summary

The MSVC specific pragmas disable this warning, but the pragmas themselves (when not guarded by any _MSC_VER ifdef) cause warnings for other targets, e.g. when targeting mingw.

Instead silence the MSVC warnings about unused parameters by casting the parameters to void.

[compnerd]

I don't have an objection to the change, but would like more details added to the commit message. It is unclear to me why the change is needed as clang does accept this I believe (and a quick test seems to indicate so as well).

[mstorsjo]

In D91851#2411671, @compnerd wrote:

I don't have an objection to the change, but would like more details added to the commit message. It is unclear to me why the change is needed as clang does accept this I believe (and a quick test seems to indicate so as well).

While clang does support those pragmas, it only recognizes them in MSVC(/itanium) mode, it doesn't in mingw mode, where I currently get these warnings:

../lib/builtins/emutls.c:185:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(push)
        ^
../lib/builtins/emutls.c:186:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(disable : 4100)
        ^
../lib/builtins/emutls.c:254:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(pop)
        ^

I can amend the commit message to something similar to this.

[compnerd]

Would be nice to actually mention that in the commit message and adjust the condition to indicate that (!defined(__MINGW32__) && !defined(__MINGW64__)).

[mstorsjo]

In D91851#2412367, @compnerd wrote:

Would be nice to actually mention that in the commit message and adjust the condition to indicate that (!defined(__MINGW32__) && !defined(__MINGW64__)).

I think that's kind of inverts the logic IMO. We have an MSVC specific pragma in win32-generic code, and thus the ifdef should check for presence of _MSC_VER, not the absence of something else.

(Also FWIW, in 64 bit cases __MINGW32__ still defined, just like _WIN32 is defined on 64 bit platforms as well.)

[mstorsjo]

In D91851#2412382, @mstorsjo wrote:

In D91851#2412367, @compnerd wrote:

Would be nice to actually mention that in the commit message and adjust the condition to indicate that (!defined(__MINGW32__) && !defined(__MINGW64__)).

I think that's kind of inverts the logic IMO. We have an MSVC specific pragma in win32-generic code, and thus the ifdef should check for presence of _MSC_VER, not the absence of something else.

... and just to remind; clang in msvc mode also defines _MSC_VER, so it has the same effect as what you suggested (excluding the pragma in mingw mode), but with less inverted logic (and would also get rid of the warning for cygwin I think, if that target would end up in this codepath).

[compnerd]

clang -target x86_64-unknown-windows-itanium -x c -E - -dM <<< '' | grep _MSC_VER

This is the reason that I prefer the "inverted" logic.

[mstorsjo]

In D91851#2412605, @compnerd wrote:

clang -target x86_64-unknown-windows-itanium -x c -E - -dM <<< '' | grep _MSC_VER

This is the reason that I prefer the "inverted" logic.

Ah, I see.

However, the itanium mode doesn't actually seems to support these pragmas either:

$ cat pragma.c 
#pragma warning(push)
#pragma warning(disable : 4206) 
#pragma warning(pop)
$ bin/clang -target x86_64-windows-msvc -c pragma.c -Wall 
$ bin/clang -target x86_64-windows-gnu -c pragma.c -Wall
pragma.c:1:9: warning: unknown pragma ignored [-Wunknown-pragmas] 
#pragma warning(push)
        ^
pragma.c:2:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(disable : 4206)
        ^
pragma.c:3:9: warning: unknown pragma ignored [-Wunknown-pragmas] 
#pragma warning(pop)
        ^ 
3 warnings generated.
$ bin/clang -target x86_64-windows-itanium -c pragma.c -Wall
pragma.c:1:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(push)
        ^
pragma.c:2:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(disable : 4206)
        ^
pragma.c:3:9: warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma warning(pop)
        ^ 
3 warnings generated.

So with that in mind, the _MSC_VER check should actually be the right one, that silences the warning on itanium too?

[mstorsjo]

compiler-rt/lib/builtins/emutls.c

33

I also noticed that we have a similar case elsewhere already in the same file, which uses #if defined(_MSC_VER) && !defined(__clang__).

187

I looked up warning 4100, https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4100, which is "unreferenced formal parameter". So I would guess one could silence that also just with a couple (void)p0; (void)p1; (void)p2; in the function...

[compnerd]

compiler-rt/lib/builtins/emutls.c

33

Yeah, using #if defined(_MSC_VER) && !defined(__clang__) in place of !mingw.

187

I actually like your idea of the cast better TBH. That is something that is not specific to MSVC but other compilers can trip over as well.

[mstorsjo]

Removed the pragmas altogether and replaced with casts to void.

[compnerd]

Thanks!

compiler-rt/lib/builtins/emutls.c

 #else
 #define EMUTLS_SKIP_DESTRUCTOR_ROUNDS 0
 #endif
 
 #if defined(_MSC_VER) && !defined(__clang__)
 // MSVC raises a warning about a nonstandard extension being used for the 0
 // sized element in this array. Disable this for warn-as-error builds.
 #pragma warning(push)
 #pragma warning(disable : 4206)
 #endif
 
 typedef struct emutls_address_array {
   uintptr_t skip_destructor_rounds;
   uintptr_t size; // number of elements in the 'data' array
   void *data[];
 } emutls_address_array;
 
static void emutls_exit(void) {
   }
   if (emutls_tls_index != TLS_OUT_OF_INDEXES) {
     emutls_shutdown((emutls_address_array *)TlsGetValue(emutls_tls_index));
     TlsFree(emutls_tls_index);
     emutls_tls_index = TLS_OUT_OF_INDEXES;
   }
 }
 
+#if defined(_MSC_VER)
 #pragma warning(push)
 #pragma warning(disable : 4100)
+#endif
 static BOOL CALLBACK emutls_init(PINIT_ONCE p0, PVOID p1, PVOID *p2) {
   emutls_mutex =
       (LPCRITICAL_SECTION)_aligned_malloc(sizeof(CRITICAL_SECTION), 16);
   if (!emutls_mutex) {
     win_error(GetLastError(), "_aligned_malloc");
     return FALSE;
   }
   InitializeCriticalSection(emutls_mutex);

 
 static __inline void __atomic_store_n(void *ptr, uintptr_t val, unsigned type) {
   assert(type == __ATOMIC_RELEASE);
   InterlockedExchangePointer((void *volatile *)ptr, (void *)val);
 }
 
 #endif // __ATOMIC_RELEASE
 
+#if defined(_MSC_VER)
 #pragma warning(pop)
+#endif
 
 #endif // _WIN32
 
 static size_t emutls_num_object = 0; // number of allocated TLS objects
 
 // Free the allocated TLS data
 static void emutls_shutdown(emutls_address_array *array) {
   if (array) {