New issue
Advanced search Search tips

Issue 618346 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

GN: unknown warning option '-Wno-maybe-uninitialized'

Project Member Reported by steve...@chromium.org, Jun 8 2016

Issue description

We are seeing warnings such as the following when building v8:

[14184/23376] CXX v8_snapshot/obj/v8/v8_base/mark-compact.o
warning: unknown warning option '-Wno-maybe-uninitialized'; did you mean '-Wno-uninitialized'? [-Wunknown-warning-option]
1 warning generated.

These are not fatal but will generate noise in the build logs.
 
Status: Fixed (was: Assigned)
Maybe this was all fixed w/ all of the flag changes?
I saw this recently. It's non fatal. I will check to see if it is still happening.

Status: Assigned (was: Fixed)
This is not fixed.

ok, but I dont see these on daisy.
what board are you using?

amd64-generic

Also arm-generic and x86-generic.

Note: I am using the prebuilts, i.e. not using the --chroot arg, but I did rerun 'gn gen out_$SDK_BOARD/Release --args="$GN_ARGS"'

Hmm. 'daisy' doesn't build for me with tot at all, looks like an afdo issue. I will file a separate bug.



Owner: dpranke@chromium.org
looked at this problem. 
it comes from host compiles with clang.
Most of them from host compiles into v8_snapshot. 
For ChromeOS simple chrome workflow, we are using GCC for target compiles and the chrome bundled clang for host compiles.
I found some places in GN where this option is added for GCC compiles.
https://cs.chromium.org/search/?q=Wno-maybe-uninitialized&sq=package:chromium&type=cs
It seems that GN is getting confused because we use GCC for target and clang for host. 
On GYP it seems to me this option was only added for target for GCC on target compiles. Maybe that is the easiest fix on GN?

Assigning to dpranke for a fix. 
I thought we were turning off clang for the host toolchain?

It looks like the v8_snapshot toolchain isn't looking at cros_host_is_clang (or any is_clang), so, yeah, it would use clang. That's an easy enough fix.

Is this showing up on a builder somewhere?
Owner: llozano@chromium.org
Actually, there is already a flag for this. Are we setting cros_v8_snapshot_is_clang=false in the $GN_ARGS ?
Yes, now that we are using GN in the SimpleChromeWorkflow stage of the PFQ
builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/amd64-generic-chromium-pfq/builds/8791/steps/SimpleChromeWorkflow/logs/stdio
@stevenjb - unfortunately, that doesn't appear to actually log the $GN_ARGS ?
never mind, I just can't read.

Yup, add cros_v8_snapshot_is_clang=false.

Also, it looks like some of the cros_ args are getting set twice?
sigh. still can't read; ignore the part about args getting set twice.

Okay, I thought we didn't want to use clang (or the chromium toolchains) anywhere, i.e., the whole point was to use gcc across the board so that you only had to worry about one toolchain?

Should these things be switched to gcc? 

I agree it's possibly we're incorrectly adding -Wno-maybe-uninitialized; I'll look at that. Feel free to re-assign to me once you clarify if clang is intentional or not.
yes, clang for host compiles in intentional. 
Unfortunately the chromeos host compiler only works within the chroot (some library issues). 
So, we have to use the clang from chromium and we cannot use clang from the host. 
For target compiles we use the compiler from ChromeOS.

I am not sure why this option is even added. v8_snapshot is not passing -Wall, and my understanding was that -Wall did not imply -Wmaybe-unitialized. 
Anyway, there must be a reason for this.

As I said, maybe the simplest fix will be to just only add this options for target compilations? 


Blocking: 433082
Cc: -dpranke@chromium.org llozano@chromium.org
Components: Build
Labels: -Build-Tools-GN Proj-GN-Migration
Owner: dpranke@chromium.org
Okay, I'll work on it, then.
Blocking: -433082
As suggested by laforge@ and a conversation w/ the monorail folks, I'm going to try tracking GN-Migration related issues by *just* using the Proj-GN-Migration label, and not using blocking/rollup bugs, so that we can use blocking for just tasks that truly need to be completed before other tasks can make progress.
Labels: -Pri-2 Pri-1
Cc: dpranke@chromium.org
Owner: llozano@chromium.org
Okay, I finally got a chance to look at this.

We should be setting -Wall when building the v8 snapshot, because it's chromium code (and, so, -Wall is on by default).

-Wno-maybe-uninitialized gets set when we think this is a gcc build (i.e., is_clang=false) and when the v8_snapshot flags aren't overridding that.

So, as I said in comment #13, we just need to add `cros_v8_snapshot_is_clang=false` to the list of args that are getting set in the environment.

@llozano - can you do this?

I looked at https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild#321 which seems to be where you would add that flag, but I don't understand how that keys off of host vs. target?
yes, I will work on this. 
Remember that these are not fatal so I dont think this needs to be a P1.

whoops, you should set cros_v8_snapshot_is_clang=true when using clang on the host toolchains, of course, since the v8_snapshot runs on the host.
Project Member

Comment 22 by bugdroid1@chromium.org, Jul 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/6da947426f1479bd148fcec6543eb5baac1ea401

commit 6da947426f1479bd148fcec6543eb5baac1ea401
Author: Luis Lozano <llozano@chromium.org>
Date: Fri Jul 22 00:43:41 2016

GN: Fix spurious compiler warning in Simple Chrome.

BUG= chromium:618346 
TEST=verify by hand we don't get the warning anymore.

Change-Id: Iecf5f68d611b293c8366c5159b886cfa414a66ac
Reviewed-on: https://chromium-review.googlesource.com/362500
Commit-Ready: Luis Lozano <llozano@chromium.org>
Tested-by: Luis Lozano <llozano@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/6da947426f1479bd148fcec6543eb5baac1ea401/cli/cros/cros_chrome_sdk.py

Status: Fixed (was: Assigned)
I think this is fixed now.
Labels: VerifyIn-54
Labels: VerifyIn-55

Comment 26 by dchan@chromium.org, Oct 10 2016

Labels: -VerifyIn-55

Comment 27 by dchan@google.com, Nov 19 2016

Labels: VerifyIn-56

Comment 28 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 29 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58
Status: Verified (was: Fixed)

Sign in to add a comment