Closed Bug 1256750 Opened 4 years ago Closed 4 years ago

media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp(690): warning C4474: 'sscanf_s' : too many arguments passed for format string

Categories

(Core :: WebRTC: Signaling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

This warning is turned into an error when building with Visual Studio 2015 Update 1 in automation.
As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings, obviously. However,
this warning occurs in 3rd party WebRTC code, so our hands are somewhat
tied. There is precedence for allowing compiler warnings in other
WebRTC projects. I guess signaling was just lucky in that it had none.

Review commit: https://reviewboard.mozilla.org/r/40181/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40181/
Attachment #8730839 - Flags: review?(rjesup)
Comment on attachment 8730839 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/1-2/
This is similar to the previous patch except it applies to our C++ in
tree as opposed to the GYP config. The same issue with the same header
is encountered.

Review commit: https://reviewboard.mozilla.org/r/40187/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40187/
Attachment #8730846 - Flags: review?(rjesup)
the sizeof(unsigned*) is not needed for windows (that's only for strings and chars and arrays).  See https://msdn.microsoft.com/en-us/library/t6z7bya3.aspx

Byron, can we just fix this the right way?
Flags: needinfo?(docfaraday)
Sure, why not.
Flags: needinfo?(docfaraday)
If we take it away we can simply make it use sscanf() on all platforms, or?
(In reply to Nils Ohlmeier [:drno] from comment #6)
> If we take it away we can simply make it use sscanf() on all platforms, or?

Hmm, it seems that the way we're using it, there is no difference between the two functions. Probably?
There is one remaining difference actually; %x is used for windows, %xu for everything else, but I don't think the latter is actually what was intended. I'm pretty sure that scans for a hex number, followed by a 'u'.
Comment on attachment 8730861 [details]
MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40195/diff/1-2/
Attachment #8730861 - Attachment description: MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows. r?jesup → MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup
Attachment #8730861 - Flags: review?(rjesup) → review+
Comment on attachment 8730861 [details]
MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup

https://reviewboard.mozilla.org/r/40195/#review36725
Comment on attachment 8730839 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/2-3/
Comment on attachment 8730846 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40187/diff/1-2/
Comment on attachment 8730839 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/3-4/
Comment on attachment 8730846 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40187/diff/2-3/
gps: what's going on with these review updates?  Is the r+'d patch to fix the root cause of the warning insufficient?
Flags: needinfo?(gps)
Comment on attachment 8730839 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/4-5/
Comment on attachment 8730846 [details]
MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40187/diff/3-4/
I keep finding the same warning from the header creep into additional compilation units (like C++ tests). Please look at the latest versions of the diffs.
Flags: needinfo?(gps)
gps: can you please provide us with the locations of your new findings so we can try to fix them rather disabling the compiler warning in lots of places?
Note: I doubt that most of us have access to VS2015 so we can't easily locate them our self.
Flags: needinfo?(gps)
I'm pretty sure it is all the same warning that is in the bug summary - we just get it from compiling a number of different things (like the tests).

Here are links to some try failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=18071349&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=18064736&repo=try
Flags: needinfo?(gps)
If it's from the same file/line, this fix should get them all.  The second one is something entirely different.  We'd far rather get a list of all the failures like this and fix them than a blanket disable of warnings/wanrings-as-errors
Rank: 23
Priority: -- → P2
Hopefully this Try push won't insta fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adaa17566ea1
Comment on attachment 8730861 [details]
MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup

The try push for this looks good (ignore the failure on 64-bit - I'm 95% confident I saw this failure on 32-bit builds and those were green). I'll withdrawal my review requests.
Attachment #8730861 - Flags: feedback+
Attachment #8730839 - Attachment is obsolete: true
Attachment #8730839 - Flags: review?(rjesup)
Attachment #8730846 - Attachment is obsolete: true
Attachment #8730846 - Flags: review?(rjesup)
Are you going to land this patch?
Flags: needinfo?(docfaraday)
This is landing.
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/6e2f198c3cfd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.