Last Comment Bug 776079 - Many -Wsometimes-uninitialized warnings in xptcinvoke_x86_64_unix.cpp
: Many -Wsometimes-uninitialized warnings in xptcinvoke_x86_64_unix.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
:
Mentors:
Depends on:
Blocks: clang
  Show dependency treegraph
 
Reported: 2012-07-20 13:15 PDT by Gregory Szorc [:gps]
Modified: 2012-07-27 08:59 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Always load (3.42 KB, patch)
2012-07-24 12:19 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
timeless: review+
respindola: checkin+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-07-20 13:15:57 PDT
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] 2012-07-20 13:46:21 PDT
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) 2012-07-20 15:28:45 PDT
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] 2012-07-20 23:52:43 PDT
Or inline, but I don't think this is a good idea.
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-07-23 06:24:57 PDT
(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) 2012-07-24 12:19:58 PDT
Created attachment 645424 [details] [diff] [review]
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 2012-07-26 11:59:52 PDT
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) 2012-07-26 13:13:56 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.