Make the C4099 warning be treated as an error on MSVC

RESOLVED FIXED in mozilla33

Status

Firefox Build System
General
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: Ehsan, Assigned: poiru)

Tracking

unspecified
mozilla33
All
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Birunthan, is this something that would interest you by any chance?
Flags: needinfo?(birunthan)
(Assignee)

Comment 2

4 years ago
(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)

Updated

4 years ago
Assignee: nobody → birunthan
(Assignee)

Comment 3

4 years ago
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.

By the way, should we revert bug 780474?
Attachment #8442506 - Flags: review?(ehsan)
(Reporter)

Comment 4

4 years ago
(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...
(Reporter)

Updated

4 years ago
Attachment #8442506 - Flags: review?(ehsan) → review+
(Reporter)

Comment 5

4 years ago
FWIW I think we should consider making this warning an error on all of our compilers.
Blocks: 780474
(Assignee)

Comment 8

4 years ago
Created attachment 8442862 [details] [diff] [review]
Disable C4099 warning in 3rd party code
Attachment #8442862 - Flags: review?(ehsan)
(Assignee)

Comment 9

4 years ago
Created attachment 8442866 [details] [diff] [review]
Re-enable -Wmismatched-tags/C4099 warnings

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)
(Reporter)

Comment 10

4 years ago
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+
(Reporter)

Comment 11

4 years ago
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+
(Assignee)

Comment 12

4 years ago
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> Thanks a lot for driving this through!

Sure thing.
Keywords: leave-open → checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93be174c5823
https://hg.mozilla.org/mozilla-central/rev/896467b67607
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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 )

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.