nsWildCard.cpp:332:22: error: unsequenced modification and access to 'y' [-Werror,-Wunsequenced]

RESOLVED FIXED in Firefox 39

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

A try push of mine [1] hit a build failure on OS X builds only. The compiler (I think correctly) pointed out unsequenced operations on a variable which can result in compiler-dependent final behaviour. The puzzling bit is that this error reproduced on my patches which have nothing to do with this code at all. I can reproduce this locally, and the specific change that causes this error to show up is the one at [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc93861714d
[2] https://hg.mozilla.org/try/rev/86500395a2db
I bisected my way through the code and the specific change that causes this error to show up is that the line of code at [1] now gets included into nsWildCard.cpp (because in my change I #included nsIWidget.h into IPCMessageUtils.h), and that's what makes the error happen.

Nathan, thoughts? I'm happy to just fix nsWildCard.cpp but I hope there isn't something more serious lurking here.

[1] http://mxr.mozilla.org/mozilla-central/source/mfbt/TypedEnumBits.h?rev=7f7f003696ad#90
Flags: needinfo?(nfroyd)
Also ni? Waldo since Nathan is away this week and this might be relevant to MFBT.
Flags: needinfo?(jwalden+bmo)
Created attachment 8572784 [details] [diff] [review]
Patch
Interesting. Some observations:

  - The line in TypedEnumBits.h defines some 'operator &&' overloads at namespace scope.

  - The line in nsWildCard.cpp contains an '&&' expression, with the two modifications 
    to 'y' being in either operand.

  - A call to the built-in 'operator &&' has short-circuit semantics, which means the
    evaluation of the first operand must be sequenced before the evaluation of the
    second.

  - A call to an overloaded 'operator &&', on the other hand, is just like any other
    function call, with the evaluations of the arguments being unsequenced.

I wonder, therefore, if this warning suggests that one of the 'operator &&' overloads is now being preferred over the built-in 'operator &&' in the line in nsWildCard.cpp. I don't see any reason why it would be (and even if it was, it would be harmless because the overloaded 'operator &&' has the same semantics as the built-in one), but that would explain the warning.

Another possibility is that for some reason, just having an overloaded 'operator &&' as a _candidate_, without it being chosen, triggers the warning.
(In reply to Botond Ballo [:botond] from comment #4)
> Another possibility is that for some reason, just having an overloaded
> 'operator &&' as a _candidate_, without it being chosen, triggers the
> warning.

Ah, I see what's happening.

  - The line in nsWildCard.cpp is inside a function template, and the
    types of operands to the '&&' expression are dependent on the template
    parameters.

  - The warning does not contain a template instantiation backtrace,
    suggesting that the warning analysis is being performed on the
    uninstantiated template code, rather than a particular
    instantiation.

  - Therefore, the warning analysis doesn't know what the concrete types
    of the operands to the '&&' are, and therefore must assume that for
    some instantiations, the overloaded 'operator &&' could be chosen.

Conclusions:

  - There is no serious issue lurking around.

  - Don't write code that modifies the same variable multiple times
    in the same statement :)
Flags: needinfo?(nfroyd)
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8572784 [details] [diff] [review]
Patch

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

Drive-by r+
Attachment #8572784 - Flags: review+
Comment on attachment 8572784 [details] [diff] [review]
Patch

Thanks! I'd still like to get an official XPCOM reviewer to review this even though it's a trivial patch.
Attachment #8572784 - Flags: review?(benjamin)
Assignee: nobody → bugmail.mozilla

Comment 8

3 years ago
Comment on attachment 8572784 [details] [diff] [review]
Patch

Good god.
Attachment #8572784 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1fca446b7e
https://hg.mozilla.org/mozilla-central/rev/1d1fca446b7e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.