776079 - Many -Wsometimes-uninitialized warnings in xptcinvoke_x86_64_unix.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gregory Szorc [:gps]]

xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp has a large number of related compiler warnings on Clang SVN HEAD that look like:

xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:126:17 [-Wsometimes-uninitialized] variable 'd7' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:127:17 [-Wsometimes-uninitialized] variable 'd6' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:127:17 [-Wsometimes-uninitialized] variable 'd7' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:128:17 [-Wsometimes-uninitialized] variable 'd5' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:128:17 [-Wsometimes-uninitialized] variable 'd6' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:128:17 [-Wsometimes-uninitialized] variable 'd7' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:129:17 [-Wsometimes-uninitialized] variable 'd4' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:129:17 [-Wsometimes-uninitialized] variable 'd5' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:129:17 [-Wsometimes-uninitialized] variable 'd6' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:129:17 [-Wsometimes-uninitialized] variable 'd7' is used uninitialized whenever switch case is taken
xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:130:17 [-Wsometimes-uninitialized] variable 'd3' is used uninitialized whenever switch case is taken

There are 57 in total. There are currently ~812 warnings when building Firefox on OS X. Removing these would remove ~7% of all compiler warnings. Not bad.

Comment 1

[Mike Hommey [:glandium]]

That's one of those cases where the warning is useless because it's done on purpose. Better disable it for this file.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Well, this particular function is best implemented in a .asm file on the side. Its use of __builtin_alloca looks very brittle.

Comment 3

[Mike Hommey [:glandium]]

Or inline, but I don't think this is a good idea.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Mike Hommey [:glandium] from comment #3)
> Or inline, but I don't think this is a good idea.

There is no point in using inline assembly to write an entire function. Do you think  that using an asm file is a bad idea? Why? The current implementation is brittle as the compiler has no reason to put that alloca where the ABI says parameters should be passed.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Always load

I decided to take a look at the code gcc and clang were generating. Both produce a jump table for each switch, which complicates the code to just avoid some loads. The code that does the call itself is much simpler than the one that does the ABI related computations, so I think we can just do the loads unconditionally.

This patch reduces the code size, fixes the warning and looks like it has no impact on performance:

http://bit.ly/MFKbLo

I still think we should rewrite this in assembly, but this is an easy drive by fix :-)

Comment 6

[timeless]

Comment on attachment 645424 [details] [diff] [review]
Always load

I wouldn't cry if there was a comment explaining that this is a UMC (Uninitialized Memory Copy) within these functions because we aren't bounds checking fpregs/gpregs against nr_fpr/nr_gpr, but explaining that a "smart" checker will recognize that we aren't using those values for anything and thus discard/suppress the warning. (Even Purify's default behavior did this, as opposed to simply reporting them as UMR.)

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 645424 [details] [diff] [review]
Always load

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=727721e5d8ac

For the record, on linux with gcc 4.7 the text size goes from 862 to 633.

Comment 8

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/727721e5d8ac