Closed Bug 1125371 Opened 5 years ago Closed 5 years ago

Silence some GCC-only warnings about uninitialized variables when building with --enable-optimize

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

There's no really satisfying fix for this kind of thing. My solution is to initialize the stupid variables, redundantly, iff the compiler is GCC.

Resisted the urge to call the macro MOZ_SHUT_UP_WESLEY().
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Thanks for doing this.  FWIW, none of the initializations in src/asmjs should affect perf so I'd be happy if you just wanted to initialize them always (which is a little less noisy-looking).  Alternatively, if you want to record that a variable's init value is just being used to silence warnings (so some wise guy doesn't come along later and remove it), how about:
  int i = MOZ_SILENCE_UNDEF_WARNING(0);
which is a bit more compact.
Comment on attachment 8553968 [details] [diff] [review]
Silence some GCC-only warnings about uninitialized varaibles when building with --enable-optimize

Review of attachment 8553968 [details] [diff] [review]:
-----------------------------------------------------------------

(thanks for doing this)

::: js/src/jit/LiveRangeAllocator.cpp
@@ +60,5 @@
>        case MUST_REUSE_INPUT:
>          n = JS_snprintf(cursor, end - cursor, "v%u", virtualRegister());
>          break;
> +      default:
> +        MOZ_CRASH("impossible Requirement::Kind");

the switch over the enum treats all existing cases. It'd be nice to just initialize n to -1 above, so that if we ever add a new value to this enum, the compiler complains that it isn't handled in this switch statement.
Comment on attachment 8553968 [details] [diff] [review]
Silence some GCC-only warnings about uninitialized varaibles when building with --enable-optimize

Review of attachment 8553968 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Looks good, although I agree with Luke that always initializing seems simpler and shouldn't affect performance.

r=me either way.
Attachment #8553968 - Flags: review?(jdemooij) → review+
(In reply to Luke Wagner [:luke] from comment #2)
> Thanks for doing this.  FWIW, none of the initializations in src/asmjs
> should affect perf so I'd be happy if you just wanted to initialize them
> always (which is a little less noisy-looking).

The reason I did it that way is so that other, more cautious compilers would have a chance to tell us if the uninitialized variable is ever used. But I guess we can do without that.

>   int i = MOZ_SILENCE_UNDEF_WARNING(0);

But it'd have to be more like

    int i MOZ_SILENCE_UNDEF_WARNING(= 0);

which makes my eyes hurt.

Plain old initialization it is. I added comments saying stuff like:

    uint32_t newLength = 0;  // initialize to silence GCC warning

Compiler weirdness causes junk to appear in your code, episode 182...
Oh, I didn't explain my idea well, I mean that:
  #define MOZ_SILENCE_UNDEF_WARNING(i) i
so that the macro was the comment, but I suppose the comment works just as well and less macros is better macros.
https://hg.mozilla.org/mozilla-central/rev/ead7aa880dbe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.