Page MenuHomePhabricator

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

Authored by mstorsjo on Fri, Nov 20, 1:41 AM.

Details

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.

Diff Detail

Event Timeline

mstorsjo created this revision.Fri, Nov 20, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 20, 1:41 AM
Herald added subscribers: Restricted Project, dberris. · View Herald Transcript
mstorsjo requested review of this revision.Fri, Nov 20, 1:41 AM

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).

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.

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

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.)

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).

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

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

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 added inline comments.
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__).

184–185

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 added inline comments.Wed, Nov 25, 8:32 AM
compiler-rt/lib/builtins/emutls.c
33

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

184–185

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 updated this revision to Diff 307700.Wed, Nov 25, 1:10 PM
mstorsjo retitled this revision from [compiler-rt] [emutls] Add ifdefs around msvc specific pragmas to [compiler-rt] [emutls] Handle unused parameters in a compiler agnostic way.
mstorsjo edited the summary of this revision. (Show Details)

Removed the pragmas altogether and replaced with casts to void.

compnerd accepted this revision.Mon, Nov 30, 9:07 AM

Thanks!

This revision is now accepted and ready to land.Mon, Nov 30, 9:07 AM