Closed Bug 1026535 Opened 6 years ago Closed 6 years ago

Make the C4099 warning be treated as an error on MSVC

Categories

(Firefox Build System :: General, defect)

All
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: ehsan, Assigned: poiru)

References

Details

Attachments

(3 files)

http://msdn.microsoft.com/en-us/library/695x5bes.aspx

MSVC uses different manglings for classes versus structs, which means if you forward declare something as a struct which is declared elsewhere as a class or vice versa you'll get a linker error.

We should really start treating warning C4099 as an error.  This is a huge footgun.
Birunthan, is this something that would interest you by any chance?
Flags: needinfo?(birunthan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #1)
> Birunthan, is this something that would interest you by any chance?

Sure, I can take a look.
Status: NEW → ASSIGNED
Flags: needinfo?(birunthan)
OS: Mac OS X → Windows 7
Hardware: x86 → All
Assignee: nobody → birunthan
Try push: https://tbpl.mozilla.org/?tree=Try&rev=0c25fd2618eb

I'll leave actually making C4099 an error for a subsequent patch due to build bustage on try.

By the way, should we revert bug 780474?
Attachment #8442506 - Flags: review?(ehsan)
(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Created attachment 8442506 [details] [diff] [review]
> Fix mismatched class/struct tags
> 
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=0c25fd2618eb
> 
> I'll leave actually making C4099 an error for a subsequent patch due to
> build bustage on try.

Sounds good.

> By the way, should we revert bug 780474?

OMG, yes.  /me goes to hang himself...
Attachment #8442506 - Flags: review?(ehsan) → review+
FWIW I think we should consider making this warning an error on all of our compilers.
Blocks: 780474
Try push: https://tbpl.mozilla.org/?tree=Try&rev=bbd190976cd1

With VC10, making C4099 an error first and then disabling it still causes an error. For example, this fails on VC10:

  // cl file.cpp -Wall -we4099 -wd4099
  class Test;
  struct Test {};
  int main() {}

On VC11/VC12, this compiles fine. So, if we want to globally[0] use -we4099, we will first need to fix 3rd party code (namely, libstagefright and protobuf). Or we could just wait until we switch to VC12. Which would you prefer?

[0]: http://dxr.mozilla.org/mozilla-central/source/configure.in#2145
Attachment #8442866 - Flags: review?(ehsan)
Comment on attachment 8442862 [details] [diff] [review]
Disable C4099 warning in 3rd party code

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

Hmm, this is a bit sad, but I can live with it.  But if you think there is a chance of getting this stuff fixed in upstream, please file bugs on them?
Attachment #8442862 - Flags: review?(ehsan) → review+
Comment on attachment 8442866 [details] [diff] [review]
Re-enable -Wmismatched-tags/C4099 warnings

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

Thanks a lot for driving this through!
Attachment #8442866 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> Thanks a lot for driving this through!

Sure thing.
https://hg.mozilla.org/mozilla-central/rev/93be174c5823
https://hg.mozilla.org/mozilla-central/rev/896467b67607
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1031978
Apologies; I mis-pasted this bug's bug number into my commit message for an unrelated patch. I backed it out & re-landed with correct bug number, but I'm noting it here in case there ends up being a sheriff-merge-tool auto-post here when the faulty commit is merged. (or in case someone ends up on this bug when doing hg archeology from my mistaken commit)

For the record, the cset with mis-pasted bug number was:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/577d7660ea9a
which I backed out in:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0351a28ceb9

(and then re-landed with fixed bug number as https://hg.mozilla.org/integration/mozilla-inbound/rev/dbfa7096402f )
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.