Remove useless assertion (which is breaking the build with gcc 6.2-7.0 and --enable-warnings-as-errors)

RESOLVED FIXED in Firefox 51

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
gcc 7.0 considers that MOZ_ASSERT(bool(&object)); in WebGLContext::ValidateObjectRef
is always going to be true.
In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:18:0,
                 from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/CheckedInt.h:13,
                 from /root/firefox-gcc-last/dom/canvas/WebGLContext.h:14,
                 from /root/firefox-gcc-last/dom/canvas/WebGL2Context.h:9,
                 from /root/firefox-gcc-last/dom/canvas/WebGL2ContextBuffers.cpp:6,
                 from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dom/canvas/Unified_cpp_dom_canvas1.cpp:2:
/root/firefox-gcc-last/dom/canvas/WebGLContext.h: In instantiation of 'bool mozilla::WebGLContext::ValidateObjectRef(const char*, const ObjectType&) [with ObjectType = mozilla::WebGLProgram]':
/root/firefox-gcc-last/dom/canvas/WebGL2ContextPrograms.cpp:22:64:   required from here
/root/firefox-gcc-last/dom/canvas/WebGLContext.h:2025:16: error: the compiler can assume that the address of 'object' will never be NULL [-Werror=address]
     MOZ_ASSERT(bool(&object));
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Likely.h:17:48: note: in definition of macro 'MOZ_UNLIKELY'
 #  define MOZ_UNLIKELY(x) (__builtin_expect(!!(x), 0))
                                                ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:377:23: note: in expansion of macro 'MOZ_CHECK_ASSERT_ASSIGNMENT'
     if (MOZ_UNLIKELY(!MOZ_CHECK_ASSERT_ASSIGNMENT(expr))) { \
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:394:39: note: in expansion of macro 'MOZ_ASSERT_HELPER1'
 #define MOZ_RELEASE_ASSERT_GLUE(a, b) a b
                                       ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:396:3: note: in expansion of macro 'MOZ_RELEASE_ASSERT_GLUE'
   MOZ_RELEASE_ASSERT_GLUE( \
   ^~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:401:27: note: in expansion of macro 'MOZ_RELEASE_ASSERT'
 #  define MOZ_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)
                           ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/dom/canvas/WebGLContext.h:2025:5: note: in expansion of macro 'MOZ_ASSERT'
     MOZ_ASSERT(bool(&object));
     ^
(Assignee)

Updated

2 years ago
Summary: Remove useless assertion (which is breaking the build with gcc 7.0) → Remove useless assertion (which is breaking the build with gcc 7.0 and --enable-warnings-as-errors)
Comment hidden (mozreview-request)

Comment 2

2 years ago
This also seems to affect GCC 6.2 as provided by Fedora 24.
Status: NEW → ASSIGNED
Summary: Remove useless assertion (which is breaking the build with gcc 7.0 and --enable-warnings-as-errors) → Remove useless assertion (which is breaking the build with gcc 6.2-7.0 and --enable-warnings-as-errors)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8815207 [details]
Bug 1320892 - Remove useless assertion (which is breaking the build with gcc 7.0 and --enable-warnings-as-errors)

https://reviewboard.mozilla.org/r/96242/#review96576

It's not useless, it's ineffective. This is a good sanity check to have, since it's quite easy to accidentally pass a null reference.
Attachment #8815207 - Flags: review?(jgilbert) → review+

Comment 4

2 years ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/616c1cdadade
Remove useless assertion (which is breaking the build with gcc 7.0 and --enable-warnings-as-errors) r=jgilbert

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/616c1cdadade
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
status-firefox51: --- → affected
status-firefox52: --- → affected
Comment on attachment 8815207 [details]
Bug 1320892 - Remove useless assertion (which is breaking the build with gcc 7.0 and --enable-warnings-as-errors)

Approval Request Comment
[Feature/Bug causing the regression]: webgl2
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:
Attachment #8815207 - Flags: approval-mozilla-beta?
Attachment #8815207 - Flags: approval-mozilla-aurora?
Comment on attachment 8815207 [details]
Bug 1320892 - Remove useless assertion (which is breaking the build with gcc 7.0 and --enable-warnings-as-errors)

WebGL2 related patch. Beta51+ & Aurora52+. Should be in 51 beta 8.
Attachment #8815207 - Flags: approval-mozilla-beta?
Attachment #8815207 - Flags: approval-mozilla-beta+
Attachment #8815207 - Flags: approval-mozilla-aurora?
Attachment #8815207 - Flags: approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(sledru)
(Assignee)

Comment 9

2 years ago
Sure but, Jeff, could you explain why you need it before I spend time on it?
Flags: needinfo?(sledru) → needinfo?(jgilbert)
https://hg.mozilla.org/releases/mozilla-aurora/rev/01baa08ef776
status-firefox52: affected → fixed
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.