The default bug view has changed. See this FAQ.

Many -Wsometimes-uninitialized warnings in xptcinvoke_x86_64_unix.cpp

RESOLVED FIXED in mozilla17

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: espindola)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
That's one of those cases where the warning is useless because it's done on purpose. Better disable it for this file.
Well, this particular function is best implemented in a .asm file on the side. Its use of __builtin_alloca looks very brittle.
Or inline, but I don't think this is a good idea.
(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.
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 :-)
Attachment #645424 - Flags: review?(benjamin)
Attachment #645424 - Flags: review?(benjamin) → review?(timeless)

Updated

5 years ago
Summary: Many -Wsomtimes-uninitialized warnings in xptcinvoke_x86_64_unix.cpp → Many -Wsometimes-uninitialized warnings in xptcinvoke_x86_64_unix.cpp

Updated

5 years ago
Assignee: nobody → respindola

Comment 6

5 years ago
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.)
Attachment #645424 - Flags: review?(timeless) → review+
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.
Attachment #645424 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/727721e5d8ac
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.