New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 771752 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

../../third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp:59:24: warning: pragma diagnostic pop could not pop, no matching push [-Wunknown-pragmas]

Project Member Reported by erikc...@chromium.org, Oct 4 2017

Issue description

[ 22170/25096 - 300 processes @ 44.7/s : 496.229s ]CXX obj/third_party/WebKit/Source/modules/media_controls/media_controls/MediaControlsResourceLoader.o
../../third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp:59:24: warning: pragma diagnostic pop could not pop, no matching push [-Wunknown-pragmas]
#pragma GCC diagnostic pop
 
Cc: beccahughes@chromium.org
 Issue 771796  has been merged into this issue.
Cc: thestig@chromium.org mgiuca@chromium.org roc...@chromium.org
Components: Blink>Media>Controls
Labels: OS-Linux
There's some discussion about this on https://chromium-review.googlesource.com/c/chromium/src/+/695328/7/third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp

Ken wrote:
"""This is a warning which for some reason we do not treat as an error, but I think it definitely needs to be fixed.

The issue is that there ought to be a corresponding push above the previous diagnostic pragma. i.e.:

#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wunknown-pragmas"


otherwise popping here makes no sense.
"""

Note that this only started happening after r506384 landed (which changed the "diagnostic ignored" to "diagnostic warning"). I don't know why. But Ken's suggestion fixes the warning (add "#pragma GCC diagnostic push" to line 40).

beccahughes@ are you able to land this fix soon?
I will point out that if this builds without the corresponding push, that means that you don't need the pop at all. Given that the warning you're trying to supress is for "unknown-pragmas", and I don't see any pragmas, this seems unnecessary. I'm guessing this was included when "#pragma warning(default : 4068)" was still compiled on non-WIN platforms.
Yeah, I'm a bit confused about what this pragma is solving since there don't seem to be any pragmas in between.

I guess a diagnostic pop without a push counts as a -Wunknown-pragmas warning, which is why it wasn't being flagged as an issue when it was set to ignore. If #3 is correct that this was only put there to work around the Microsoft-specific pragma on non-Windows platforms, then this shouldn't be necessary given that those are in an OS_WIN condition.

BTW, here is GCC's manual page on the subject:
https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
(I know we're using Clang, but I assume they implemented the same thing.)
The pragmas only appear for internal builds when resource whitelisting is turned on. Grit injects a pragma into the resource header files which then throws an "unknown pragma" warning on compile time. Grit will then run generate_resource_whitelist.py which will grep over the build logs looking for those warnings. Any resource that does not have a warning is stripped from the final build. This then caused crashes since resources were missing from Chrome. We compile Blink with "-Werror" so these warning also cause build failures, hence the pragma to output them as warnings and not errors.

The "pragma diagnostic pop" warning is a bug though and I will fix that.
#5 makes sense. I'm not sure, though, why this isn't an error compiling Chromium, since I thought we compiled everything with -Werror. (?)

Anyway, thanks for fixing the warning.
Status: Started (was: Assigned)
CL to fix this is out for review: https://chromium-review.googlesource.com/c/chromium/src/+/702275
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8170e999fd677b429c2b75f7a212e3087ee81a9e

commit 8170e999fd677b429c2b75f7a212e3087ee81a9e
Author: Becca Hughes <beccahughes@chromium.org>
Date: Thu Oct 05 11:37:58 2017

Fix compiler warning in Blink.

Fix "no matching push" compiler warning in Blink.

BUG= 771752 

Change-Id: I7a4b49c99c2f74cf18c03c025994284730ce9536
Reviewed-on: https://chromium-review.googlesource.com/702275
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506701}
[modify] https://crrev.com/8170e999fd677b429c2b75f7a212e3087ee81a9e/third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp

Status: Fixed (was: Started)
 Issue 771968  has been merged into this issue.

Sign in to add a comment